[PATCH v8] string: Adds tests for test-strncasecmp and test-strncpy
Raoni Fassina Firmino
raoni@linux.ibm.com
Fri Jul 31 14:14:39 GMT 2020
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
More information about the Libc-alpha
mailing list