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

[Pedro Alves <palves@redhat.com>]

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