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

Pedro Alves
2016-11-17T20:30:49Z

On 11/17/2016 07:36 PM, Andreas Arnez wrote:
> On Tue, Nov 15 2016, Pedro Alves wrote:
>
>> Looks like the sort of function that should be possible to
>> cover all sorts of inputs/outputs with unit tests. Aligned, misaligned,
>> big/little endian, etc., that sort of thing. That'd help
>> a lot with ensuring rewrites behave as intended. Would you feel like
>> including some?
>
> Sure. See the patch below, which I'd insert after the one that fixes
> copy_bitwise.

Thanks!

> With that added, is the series OK to apply?

I'm feeling dense and I can't make sense of the new test. :-/

Can you add some more comments? Is there some logic behind the
numbers of the data1/data2 arrays? Some pattern between them?
E.g., it'd be nice to explain the logic between the steps
check_copy_bitwise is doing.

A couple other minor comments:

> +#if GDB_SELF_TEST
> +

Please add:

namespace selftests {

like utils-selftests.c does.

>

> + a[len] = b[len] = '\0';
> + if (strcmp (a, b))

strcmp (...) != 0

> + error (_("copy_bitwise %s != %s (%u+%u -> %u)"),
> + a, b, source_offset, nbits, dest_offset);
> +}

> +
> + enum { max_nbits = 24, max_offs = 8 };

IN C++11 we can express a compile-time integer directly with
constexpr, not need for enum hacks any longer:

constexpr int max_nbits = 24;

Thanks,
Pedro Alves