[PATCH] D25587: Introduce llvm FormatVariadic

[Bob Haarman via llvm-commits]

inglorion added a comment.

I have a couple of questions and comments inline. Thanks for making the changes I suggested earlier!



================
Comment at: include/llvm/Support/FormatAdapters.h:20
+namespace llvm {
+template <typename T> struct AdapterBase {
+protected:
----------------
Can you make this class instead of struct? The classes that extend it are all declared as class, and you are explicitly specifying protected anyway, so might as well make it more consistent.


================
Comment at: include/llvm/Support/FormatAdapters.h:26
+  static_assert(!detail::uses_missing_provider<T>::value,
+                "Item does not have a format provider!");
+};
----------------
Is there any way to get the name of the type in the message?


================
Comment at: include/llvm/Support/FormatCommon.h:32
+    // in order to calculate how long the output is so we can align it.
+    // TODO: Make the format method return the number of bytes written, that
+    // way we can also skip the intermediate stream for left-aligned output.
----------------
That seems like a useful change to make. Want to make it before committing this? The amount of code that is going to have to be changed for it is only going to get larger.


================
Comment at: include/llvm/Support/FormatCommon.h:45
+        size_t PadAmount = Amount - Item.size();
+        if (Where == AlignStyle::Left) {
+          S << Item;
----------------
In general for code that dispatches on an enum, if you convert it to switch instead of if, the compiler can warn if the code doesn't handle all the cases. Clang doesn't seem to do this for if ... else if, so I prefer code like this to be written using switch.


================
Comment at: include/llvm/Support/FormatProviders.h:21
+#include "llvm/ADT/StringSwitch.h"
+// #include "llvm/Support/Format.h"
+#include "llvm/Support/FormatVariadicDetails.h"
----------------
Please remove.


================
Comment at: include/llvm/Support/FormatProviders.h:32
+    : public std::integral_constant<
+          bool, is_one_of<T, uint8_t, int16_t, uint16_t, int32_t, uint32_t,
+                          int64_t, uint64_t>::value> {};
----------------
Instead of explicitly enumerating integral types, can we use something like is_integral<T>::value && !use_char_formatter<T>::value here?


================
Comment at: include/llvm/Support/FormatProviders.h:37
+struct use_char_formatter
+    : public std::integral_constant<bool, std::is_same<T, char>::value> {};
+
----------------
Should this support unsigned char, too? What about wchar_t?


================
Comment at: include/llvm/Support/FormatProviders.h:87
+///
+///   ==========================================================================
+///   |  style  |     Meaning          |      Example     | Precision Meaning  |
----------------
For styles other than hex and exponential, is there a difference between the uppercase and lowercase specifiers? If not, consider only supporting one so that we get consistent usage in the code base.


================
Comment at: include/llvm/Support/FormatProviders.h:250
+
+/// \brief implementation of format_provider<T> for characters.
+///
----------------
In cases like these where \brief is used with a single-sentence paragraph, I would leave out the \brief - looks a little cleaner. It's also consistent with http://llvm.org/docs/CodingStandards.html#doxygen-use-in-documentation-comments


================
Comment at: lib/Support/NativeFormatting.cpp:72
+    double D = static_cast<double>(N);
+    if (IsNegative)
+      D = -D;
----------------
I don't follow - why do we have to check IsNegative when T is an unsigned type?


https://reviews.llvm.org/D25587