[PATCH 1/2] Expand bitvec interface

[Max (☭)]

04.02.2016 13:39, Holger Freyther пишет:

>> +	memcpy(tmp, bv->data, 2);
>> +	return osmo_load16be(tmp) >> (16 - num_bits);
> 
> load16be is working byte by byte as well so what do we win by this load?
> 

I don't really get this - win compared to what? This is new function, not replacement
for some old code.

>> +/*! \brief fill num_bits with \fill starting from the current position
>> + * returns 0 on success, negative otherwise (out of vector boundary)
>> + */
>> +int bitvec_fill(struct bitvec *bv, unsigned int num_bits, enum bit_value fill)
>> +{
>> +	unsigned i, stop = bv->cur_bit + num_bits;
>> +	for (i = bv->cur_bit; i < stop; i++)
>> +		if (bitvec_set_bit(bv, fill) < 0)
>> +			return -EINVAL;
>> +
>> +	return 0;
>> +}
>> +
>> /*! \brief pad all remaining bits up to num_bits */
>> int bitvec_spare_padding(struct bitvec *bv, unsigned int up_to_bit)
>> {
>> -	unsigned int i;
>> -
>> -	for (i = bv->cur_bit; i <= up_to_bit; i++)
>> -		bitvec_set_bit(bv, L);
>> +	int n = up_to_bit - bv->cur_bit + 1;
>> +	if (n < 1)
>> +		return 0;
> 
> so we are going from unsigned to int and then 'n' is "converted" to unsigned int as well. Is this what we want?
> 

Yes. By the time we use n as unsigned we've explicitly checked already that it's
positive. On the other hand the diff between 2 unsigned ints can be signed.

> 
>> +/*! \brief convert enum to corresponding character */
>> +char bit_value_to_char(enum bit_value v)
>> +{
>> +	switch (v) {
>> +	case ZERO: return '0';
>> +	case ONE: return '1';
>> +	case L: return 'L';
>> +	case H: return 'H';
>> +	}
>> +	/* make compiler happy - "avoid control reaches end of non-void function" warning: */
> 
> seeing is believing? Which gcc? I had proposed to use  __builtin_unreachable here. If we know we don't end up there, put this into the "branch" and the warning is gone. Or abort() or __builtin_trap.
> 

I think __builtin_unreachable is deprecated but abort() in default: branch will do
the trick.

> 
> 
> 
>> +	return '?';
>> +}
>> +
>> +/*! \brief prints bit vector to provided string
>> + * It's caller's responcibility to ensure that we won't shoot him in the foot.
> 
> typo, how long is "str"? So either pass a size or write it in the comment of how long it needs to be depending on cur_bit? And what does the _r stand for? Is it similiar to the libc _r functions where an external buffer/pointer is provided?
> 

The name was suggested by Harald in earlier review. The string should be at least
cur_bit+1 bytes long.

> 
>> +/* we assume that x have at least 1 non-b bit */
>> +static inline unsigned _leading_bits(uint8_t x, bool b)
> 
> _ and __ are reserved for the system, let us not use it. By having this method as static we already don't pollute the global namespace.
> 

So how shall it be called to make it clear that this is internal to implementation/file?

> 
>> +/*! \brief Return number (bits) of uninterrupted run of \b in \bv starting from the MSB */
> 
> Are you sure that \b and \bv refer to the params? not like \param b, \param bv? Did you look at the documentation generated by doxygen?

Indeed, it should be \param, or better yet - full comment instead of brief one.

> 
> 
>> -		fprintf(stderr, "out: %s\n", osmo_hexdump(out, sizeof(out)));
>> +		printf("out: %s\n", osmo_hexdump(out, sizeof(out)));
> 
> why? more output to be tracked? we could not ignore stderr if we want to?
> 

We could but since it's tests I'd rather have it under VCS in bitvec_test.ok
explicitly. I think we only should ignore volatile things.

> 
> 
>>
>> 		OSMO_ASSERT(out[0] == 0xff);
>> 		OSMO_ASSERT(out[in_size+1] == 0xff);
>> @@ -72,11 +145,77 @@ static void test_unhex(const char *hex)
>>
>> int main(int argc, char **argv)
>> {
>> +	srand(time(NULL));
> 
> why?
>

Leftover from the time I was thinking of adding tests with random values. Can be
safely removed.

cheers,
Max.