Re: [PATCH 2/3] Fix copy_bitwise()

[Andreas Arnez <arnez@linux.vnet.ibm.com>]

On Mon, Nov 14 2016, Luis Machado wrote:

> On 11/14/2016 09:02 AM, Andreas Arnez wrote:

[...]

>> +      dest += dest_offset / 8;
>> +      dest_offset %= 8;
>> +      source += source_offset / 8;
>> +      source_offset %= 8;
>
> Are you sure you will always have non-zero source_offset and dest_offset
> when explicitly dividing them by 8? If i were to feed (or GDB, in some
> erroneous state) invalid data to the function, this would likely crash?
>
> There are other cases of explicit / operations.

No, copy_bitwise should work fine with source_offset and dest_offset set
to zero.  Where do you think it would crash?

[...]

>> +  /* Fill BUF with DEST_OFFSET bits from the destination and 8 -
>> +     SOURCE_OFFSET bits from the source.  */
>> +  buf = *(bits_big_endian ? source-- : source++) >> source_offset;
>
> Maybe it's just me, but having constructs like the above don't help much
> performance-wise and make the code slightly less readable. Should we
> expand this further? There are multiple occurrences of this.

Well, I've tried a few different ways and found this approach actually
the easiest to read, for my taste.  For instance, it makes the multiple
occurrences easy to recognize -- as you pointed out ;-)

Of course, if people feel that this post-decrement/increment pattern
really hurts readability, I can provide a more "stretched" form instead.

> Also, should we harden the method to prevent dereferencing NULL source or
> dest?

I wouldn't consider that necessary, but I'm open for other opinions.

[...]

> Otherwise it looks good to me.

Thanks!

--
Andreas