Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42311 )
Change subject: tests: Complete lib/string-test test case ......................................................................
Patch Set 1:
(9 comments)
https://review.coreboot.org/c/coreboot/+/42311/1/tests/lib/string-test.c File tests/lib/string-test.c:
https://review.coreboot.org/c/coreboot/+/42311/1/tests/lib/string-test.c@a10 PS1, Line 10: One can use __builtin_xxx for many of the most simple str*() : * functions, when non-coreboot one is required. Not sure why you're removing this? I think it's still useful advice...
https://review.coreboot.org/c/coreboot/+/42311/1/tests/lib/string-test.c@96 PS1, Line 96: str_len > n ? n : str_len; nit: could also be MAX(n, str_len)
https://review.coreboot.org/c/coreboot/+/42311/1/tests/lib/string-test.c@126 PS1, Line 126: assert_ptr_not_equal(str + 2, strrchr(str, 'r')); nit: This one is kinda redundant with the line above it. str + 9 could never equal str + 2. Maybe instead try to find the 'A' or the '\n' to make sure there are no edge cases with first/last character?
https://review.coreboot.org/c/coreboot/+/42311/1/tests/lib/string-test.c@135 PS1, Line 135: DST_SIZE Could just reorder the definitions and say sizeof(src) here so you don't need to hardcode a constant for this.
https://review.coreboot.org/c/coreboot/+/42311/1/tests/lib/string-test.c@139 PS1, Line 139: n nit: might also be interesting to copy less and ensure that characters behind the limit are not overwritten.
https://review.coreboot.org/c/coreboot/+/42311/1/tests/lib/string-test.c@159 PS1, Line 159: assert_true(dst[__builtin_strlen(dst)] == '\0'); This is sort of a tautology because __builtin_strlen() always finds a \0 by definition (so this will be true even if the string wasn't terminated and __builtin_strlen(dst) finds some other zero byte further down the stack). Should say __builtin_strlen(src) here to make sure the \0 is really where it is supposed to be. (Alternatively, could just add +1 to the size of the line above and memcmp() will already check that for you.)
https://review.coreboot.org/c/coreboot/+/42311/1/tests/lib/string-test.c@168 PS1, Line 168: assert_int_equal(0, strcmp(str, str)); nit: this would also be true if strcmp() just compared pointers, I think comparing two distinct arrays would be better.
https://review.coreboot.org/c/coreboot/+/42311/1/tests/lib/string-test.c@191 PS1, Line 191: } Would be good to pass a real (char **) variable and confirm that the pointer is updated to point behind the parsed number, as described in the skip_atoi() header comment.
https://review.coreboot.org/c/coreboot/+/42311/1/tests/lib/string-test.c@198 PS1, Line 198: char str3[] = "1234"; The order of these should be different than the order in the tested string, to make sure strspn() doesn't just do a strcmp() but actually treats this as a set of characters. (Also, would be better if a few of these characters appear more than once in the found segment, just to make sure strspn() doesn't try to check for each of them only once.)