This patch changes the printf parser tests to use a more robust matcher.
This allows for better debugging of parsing issues. This does not affect
the actual printf code at all, only the tests.
Details
- Reviewers
sivachandra lntue - Commits
- rGff1374785f82: [libc] Add Printf FormatSection Matcher
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
libc/utils/UnitTest/PrintfMatcher.cpp | ||
---|---|---|
18 | Instead of all the comparisons in this function, can you define operator== on FormatSection? | |
25 | We can use cpp::StringView to perform this comparison. | |
53 | We have this: https://github.com/llvm/llvm-project/blob/main/libc/utils/UnitTest/FPMatcher.cpp#L23 You can move it to a common place may be? You can use std::string in this file. | |
67 | Wrap this in a do-while? | |
118 | "Expected format section: " | |
121 | "Actual format section: " |
add a util file for int_to_hex and address other comments
libc/utils/UnitTest/PrintfMatcher.cpp | ||
---|---|---|
18 | I think it's better for final code size if FormatSection doesn't have the comparison operator since it's never used in actual printf code. | |
25 | we could create StringViews for these, but I don't see a reason to, since this is closer to the way these objects are actually used. | |
53 | I've done that, does the bazel also need an update? |
libc/utils/UnitTest/PrintfMatcher.cpp | ||
---|---|---|
18 | Unused inline functions/methods will not contribute to code size. Also, applications which care about code size will/should use LTO which will kick out all unused functions. You can add a comment along side operator== to make it clear that it is used only for testing. | |
25 | It could be used differently elsewhere but you are comparing two string views here. So, the following is much better and avoids duplication of code: if (cpp::StringView(expected.raw_string, expected.raw_len) != cpp::StringView(expected.raw_string, expected.raw_len)) return false; It also makes the conditional on line 23 simpler: if (expected.has_conv != actual.has_conv) return false; |
libc/src/stdio/printf_core/core_structs.h | ||
---|---|---|
55 | FormatSection is quite big, you should use const-ref: bool operator==(const FormatSection &other) const { ... } |
libc/utils/UnitTest/CMakeLists.txt | ||
---|---|---|
20 | Ah, if you add this with add_header_library, you will not require the empty cpp file. |
FormatSection is quite big, you should use const-ref: