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

[Akira Hatanaka via Phabricator via llvm-commits]

ahatanak added inline comments.


================
Comment at: llvm/include/llvm/Support/Format.h:183
 }
 
+template <typename... Ts>
----------------
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?


================
Comment at: llvm/lib/Support/Format.cpp:16
+
+namespace {
+enum ArgLength {
----------------
Can you leave comments that explain what these enums mean?


================
Comment at: llvm/lib/Support/Format.cpp:99
+        ST_Double,
+        ST_Unknown,
+        ST_Unknown,
----------------
I think this rejects the combination of length modifier `l` and floating point conversions (e.g., `%lf`), but the standard doesn't say that's undefined behavior. It just says it has no effect.


================
Comment at: llvm/lib/Support/Format.cpp:157
+const char *next(const char *str, char c) {
+  while (*str) {
+    if (*str == c)
----------------
Can you use `StringRef::find` or some other functions that is available instead of writing a loop here?


================
Comment at: llvm/lib/Support/Format.cpp:188
+
+  // skip flags
+  StringRef FlagChars = "+- #0";
----------------
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?


================
Comment at: llvm/lib/Support/Format.cpp:328
+  PrintfStyleFormatReader EFR(Expected);
+  PrintfStyleFormatReader FFR(Fmt);
+  while (SpecifierType ST = FFR.NextSpecifier())
----------------
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`?


================
Comment at: llvm/lib/TableGen/SetTheory.cpp:223
       raw_string_ostream OS(Name);
-      OS << format(Format.c_str(), unsigned(From));
+      OS << format_chk("<fallback: %u>", Format.c_str(), unsigned(From));
       Record *Rec = Records.getDef(OS.str());
----------------
I think it's better to immediately fail here if the format string passed at runtime is invalid and inform the user of the location of the invalid string by calling a function in Error.h (e.g., `PrintFatalError`).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132413