[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;
----------------
michaelrj wrote:
> 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.
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.


================
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]))
----------------
michaelrj wrote:
> 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.
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;
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124130