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

[Luis Machado <lgustavo@codesourcery.com>]

On 11/14/2016 11:54 AM, Andreas Arnez wrote:
> 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?
>

Well, obviously i wasn't fully awake. Nevermind this.

> [...]
>
>>> +  /* 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.
>

No strong opinions there. Just a suggestion.