Anna Karaś has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42311 )
Change subject: tests: Complete lib/string-test test case ......................................................................
Patch Set 2:
(12 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@5 PS1, Line 5: stdint.h
<stddef. […]
Done
https://review.coreboot.org/c/coreboot/+/42311/1/tests/lib/string-test.c@33 PS1, Line 33: struct str_with_l_val_t {
Consider adding comments here and for str_with_u_val_t that these are being used to test atol and at […]
Done.
https://review.coreboot.org/c/coreboot/+/42311/1/tests/lib/string-test.c@48 PS1, Line 48: uint32_t
atoi returns a signed int
I based choice of function return type on an already existing atoi implementation from src/lib/string.c - "unsigned int skip_atoi(char **s)".
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)
Done.
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. […]
Done.
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 […]
Done.
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 over […]
Done.
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 […]
Done.
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 arra […]
Done.
https://review.coreboot.org/c/coreboot/+/42311/1/tests/lib/string-test.c@190 PS1, Line 190: assert_int_equal
assert_int_equal expects signed integers. Once you fix the type for str_with_u_val. […]
Until we determine (see one of comments above) whether to keep the unsigned int type or change to signed int, I suggest temporarily replacing assert_int_equal with assert_true.
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 beh […]
I suppose that confirmation of the update is still possible with this implementation. Just add an "offset" field to str_with_u_val and compare it with a difference between updated ptr and a copy of ptr (the copy made before the update of ptr).
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() do […]
Done.