libc/tests/string: add a more comprehensive unit test for strrchr()

[fuz]

Summary

This catches the issue found in 293915.

PR: 293915
Reported by: [e-mail]

[ngie]

A lot of important details are missing from these tests in terms of your thought process in these tests doing what they do, why assertions are being made, etc. These tests aren't really maintainable in their current form longterm.

lib/libc/tests/string/strrchr_test.c

23–29

Could these please be made into constants?

43–49

Could these please be made into constants?

74–80

Could these please be made into constants?

[ngie]

lib/libc/tests/string/strrchr_test.c

76–77

Also, why X/-?

[fuz]

This test is an adaption of the memrchr test with minor modifications.

For example, one of the things I do when debugging is to temporarily change the boundaries of a loop to only the cases where I have observed a failure, allowing me to go directly to that iteration in the debugger. This sort of thing is really annoying to do when everything is spammed with symbolic constants.

lib/libc/tests/string/strrchr_test.c

23–29

I'm not sure if this would be helpful here. These numbers correspond to the possible choices in the SSE implementation, which processes 16 bytes per vector (thus 16 possibly misalignments) and 32 bytes per iteration (thus 64 bytes total length to complete the main loop at least once). If I change the code such that these values change, I plan to bump the unit test to cover the extended range.

The same applies to the other cases and in fact pretty much all the string unit tests I have written so far. I can add some arbitrary symbolic names for these numbers, but I don't see how that improves things, it just adds more clutter. If you insist, I can try and do some systematic refactoring to add symbolic constants at a later point, but it makes the unit tests less useful as a development tool for me, as it becomes harder to modify the numbers when the code changes.

My main priority is to get the fix for 293915 in right now.

76–77

The choice is arbitrary. X is the character we are looking for, - is some other character. They were chosen to be easy to distinguish visually when inspecting a test suite failure in the debugger. Unit test values goes through all possible characters to try to catch arithmetic errors in the scalar kernel (cf. 3d8ef25).

[fuz]

• libc/strrchr_test.c: explain what the unit tests test for

[strajabot]

LGTM

lib/libc/tests/string/strrchr_test.c

28

nit: I would specify alignas(16) for better repeatability. I'm not 100% convinced it won't fail on i = 4 for one version of the compiler, but 8 for another, if the stack layout is changed

[fuz]

lib/libc/tests/string/strrchr_test.c

28

nit: I would specify alignas(16) for better repeatability. I'm not 100% convinced it won't fail on i = 4 for one version of the compiler, but 8 for another, if the stack layout is changed

Indeed, but it'll cycle through all alignments anyway and the specific iteration it fails on doesn't really matter.

[fuz]

@ngie Is it better now with comments?

I don't really want to add symbolic constants as they make the unit test a whole lot less useful as a debug tool than it currently is.