[PATCH] D25587: Introduce llvm FormatVariadic

[Chandler Carruth via llvm-commits]

chandlerc added a comment.

Nitpicks I noticed when reading it. Some of it is due to the changes to remove UDLs.



================
Comment at: include/llvm/Support/FormatCommon.h:34-36
+    if (Amount == 0) {
+      Wrapper.format(S, Options);
+    } else {
----------------
Please use early return to reduce indentation.


================
Comment at: include/llvm/Support/FormatCommon.h:41-43
+      if (Amount <= Item.size()) {
+        S << Item;
+      } else {
----------------
Same here.


================
Comment at: include/llvm/Support/FormatVariadic.h:15-19
+//   // Convert to string.
+//   std::string S = "{0} {1}"_fmt.string(1234.412, "test");
+//
+//   // Stream to an existing raw_ostream.
+//   OS << "{0} {1}"_fmt.stream(1234.412, "test");
----------------
Comments still reference UDL syntax.


================
Comment at: include/llvm/Support/FormatVariadic.h:159
+
+// \brief format_string() function.
+//
----------------
This is now spelled "formatv". The entire comment block needs updating.

Also, I'd suggest not repeating the name of the thing in the brief comment. In Doxygen it is displayed right next to the name anyways so it becomes rapidly redundant and as it is here can easily fall out of sync. Instead, I'd write a one sentence summary for the brief section.


================
Comment at: include/llvm/Support/FormatVariadic.h:203
+//
+// === Parameter Indexing ===
+// `index` specifies the index of the paramter in the parameter pack to format
----------------
This header is different from all the others. Maybe use markdown syntax if you need this kind of structure?


================
Comment at: include/llvm/Support/FormatVariadic.h:239-240
+// the details of what that is are undefined.
+//
+
+template <typename... Ts>
----------------
I would prefer to skip the empty lines to make it clear what this block applies to.


https://reviews.llvm.org/D25587