[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:
> 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.
alright, in that case I'll just do it.


================
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:
> 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;
> ```
Ah, I thought `StringView` was more complicated than that. I've done that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124130