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

[Siva Chandra via Phabricator via libc-commits]

sivachandra added inline comments.


================
Comment at: libc/utils/UnitTest/PrintfMatcher.cpp:17
+
+bool FormatSectionMatcher::match(FormatSection actualValue) {
+  actual = actualValue;
----------------
Instead of all the comparisons in this function, can you define `operator==` on `FormatSection`?


================
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]))
----------------
We can use `cpp::StringView` to perform this comparison.


================
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;
----------------
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.


================
Comment at: libc/utils/UnitTest/PrintfMatcher.cpp:66
+#define IF_FLAG_SHOW_FLAG(flag_name)                                           \
+  if ((form.flags & FormatFlags::flag_name) == FormatFlags::flag_name)         \
+  stream << "\n\t\t" << #flag_name
----------------
Wrap this in a `do`-`while`?


================
Comment at: libc/utils/UnitTest/PrintfMatcher.cpp:117
+void FormatSectionMatcher::explainError(testutils::StreamWrapper &stream) {
+  stream << "expected : ";
+  display(stream, expected);
----------------
"Expected format section: "


================
Comment at: libc/utils/UnitTest/PrintfMatcher.cpp:120
+  stream << '\n';
+  stream << "actual   : ";
+  display(stream, actual);
----------------
"Actual format section: "


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124130/new/

https://reviews.llvm.org/D124130