[prev in list] [next in list] [prev in thread] [next in thread]
List: glibc-alpha
Subject: Re: [PATCH v8] string: Adds tests for test-strncasecmp and test-strncpy
From: Raoni Fassina Firmino via Libc-alpha <libc-alpha () sourceware ! org>
Date: 2020-07-31 14:14:39
Message-ID: 20200731141439.jwqewilnsfrnw266 () work-tp
[Download RAW message or body]
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
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic