[PATCH] D132413: [NFC] Make format() more amenable to format attributes

[Félix Cloutier via Phabricator via llvm-commits]

fcloutier added inline comments.


================
Comment at: llvm/include/llvm/Support/Format.h:183
 }
 
+template <typename... Ts>
----------------
ahatanak wrote:
> I don't think it's clear to the users of this function what they should pass for `Expected`. Can you leave a comment that explains what this function expects or in which cases `Expected` and `Fmt` would be incompatible?
Since there was just one use of this function and it was preferable to check a different way, I removed it entirely.


================
Comment at: llvm/lib/Support/Format.cpp:157
+const char *next(const char *str, char c) {
+  while (*str) {
+    if (*str == c)
----------------
ahatanak wrote:
> Can you use `StringRef::find` or some other functions that is available instead of writing a loop here?
I started off using `StringRef` and found it really cumbersome compared to using a raw `const char *`. In retrospective, it's not surprising given that printf was meant for C first. I removed `next` and replaced its only use with `strchr` to avoid the loop.


================
Comment at: llvm/lib/Support/Format.cpp:188
+
+  // skip flags
+  StringRef FlagChars = "+- #0";
----------------
ahatanak wrote:
> Flags are completely ignored here, but isn't the behavior undefined for some combinations of flags and conversions according to the standard (e.g., `"%0p"`)? If it is, can you leave a comment that notes that this code ignores undefined behavior? Or can you leave a FIXME note if you plan to fix this in the future?
I implemented the missing checks.


================
Comment at: llvm/lib/Support/Format.cpp:328
+  PrintfStyleFormatReader EFR(Expected);
+  PrintfStyleFormatReader FFR(Fmt);
+  while (SpecifierType ST = FFR.NextSpecifier())
----------------
ahatanak wrote:
> Don't you have to check that the lengths of both lists are equal? What happens if `Expected` is `%d%d` and `Fmt` is `%d`?
It's not UB to call `va_end` before you've read each argument of a `va_list`, so the one thing that matters is that `Fmt` doesn't have more specifiers than `Expected`. This is ensured by breaking out of the loop when `Fmt` runs out of specifiers, regardless of whether `Expected` still has more. If we run out of `Expected` specifiers while `Fmt` still has specifiers, we error out.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132413