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

Luis Machado
2016-11-14T17:58:20Z

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.