Re: [PATCH v8] string: Adds tests for test-strncasecmp and test-strncpy

[Raoni Fassina Firmino via Libc-alpha <libc-alpha@sourceware.org>]

Hi Raphael,

Sorry for my late review, hope it is useful.

I Tested the patch on powerpc-linux-gnu (with kernel ppc64) on top of
master (0ad926f349) with no problems.


> diff --git a/string/test-strncasecmp.c b/string/test-strncasecmp.c

OK.


> diff --git a/string/test-strncpy.c b/string/test-strncpy.c
...

> +  const size_t maxoffset = 64;

I am confused about the role of maxoffset == 64 and sizeof(CHAR), if the
intended value of 64 is for the buffer size in bytes or array elements.
But in any case I think It surely is not a problem to be larger than the
minimum 64 bytes, I am just pointing it out because I may be missing
something.


> +  /* Put s1 at the maxoffset from the edge of buf1's last page.  */
> +  s1 = (CHAR *) buf1 + BUF1PAGES * page_size / sizeof(CHAR) - maxoffset;

I don't understand how the position in the buffer is being calculated
here, IIUIC you want the last `maxoffset * sizeof(CHAR)` bytes at the
end of buf1, in this case `page_size / sizeof(CHAR)` doesn't make sense
for me, but my math may be off.


> +  /* s2 needs room to put a string with size of maxoffset + 1 at s2 +
> +     (maxoffset - 1).  */
> +  s2 = (CHAR *) buf2 + page_size / sizeof(CHAR) - maxoffset * 2;

Same as before, but also `maxoffset * 2` guarantees that only one of the
tests will touch the edge of buf2 page, I am not sure if this is the
intended case.


> +	    do_one_test (impl, (s2 + off2), (s1 + off1), maxoffset - off1 - 1,
> +			 maxoffset + 1);

Reading do_one_test, it seems like len, here as `maxoffset - off1 - 1`,
should be `maxoffset - off1`, as it is supposed to be the size of src.
I know that because the trailing '\0' on src, this in practice will not
be a problem, but for a case when sizeof(src) and sizeof(dest) is the
same, do_one_test will ended up doing the last bit for len < n.

nit, and totally optional, you don't need "()" around (s2 + off2) and
(s1 + off1), I am only mentioning because you didn't use it for the
other parameters.



o/
Raoni