[libc-commits] [PATCH] D124130: [libc] Add Printf FormatSection Matcher
Michael Jones via Phabricator via libc-commits
libc-commits at lists.llvm.org
Thu Apr 21 15:10:26 PDT 2022
michaelrj added inline comments.
================
Comment at: libc/utils/UnitTest/PrintfMatcher.cpp:17
+
+bool FormatSectionMatcher::match(FormatSection actualValue) {
+ actual = actualValue;
----------------
sivachandra wrote:
> Instead of all the comparisons in this function, can you define `operator==` on `FormatSection`?
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.
================
Comment at: libc/utils/UnitTest/PrintfMatcher.cpp:24
+
+ for (size_t i = 0; i < expected.raw_len; ++i) {
+ if (!(expected.raw_string[i] == actual.raw_string[i]))
----------------
sivachandra wrote:
> We can use `cpp::StringView` to perform this comparison.
we could create `StringView`s for these, but I don't see a reason to, since this is closer to the way these objects are actually used.
================
Comment at: libc/utils/UnitTest/PrintfMatcher.cpp:52
+// to buff in hex. It also returns buff for ease of use.
+char *int_to_hex(T num, char *buff) {
+ constexpr size_t len = sizeof(T) * 2;
----------------
sivachandra wrote:
> 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.
I've done that, does the bazel also need an update?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D124130/new/
https://reviews.llvm.org/D124130
More information about the libc-commits
mailing list