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.
