[libc-commits] [PATCH] D124130: [libc] Add Printf FormatSection Matcher

[Michael Jones via Phabricator via libc-commits]

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