[PATCH] D25587: Introduce llvm FormatVariadic

[Zachary Turner via llvm-commits]

zturner added inline comments.


================
Comment at: include/llvm/Support/FormatAdapters.h:26
+  static_assert(!detail::uses_missing_provider<T>::value,
+                "Item does not have a format provider!");
+};
----------------
inglorion wrote:
> Is there any way to get the name of the type in the message?
That's actually the whole reason for this.  You will get the name of the type because `T` is the type, and most compilers will print out what `T` is.


================
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.
----------------
inglorion wrote:
> 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.
I thought about it, but honestly I'd rather do it later.  It's purely an optimization, and once this is in I can make that change via post-commit review.


================
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> {};
----------------
inglorion wrote:
> Instead of explicitly enumerating integral types, can we use something like is_integral<T>::value && !use_char_formatter<T>::value here?
`is_integral` also includes `bool`.  So you'd also have to add a `!use_bool_formatter`.  At that point I think this more clearly expresses the intention.


================
Comment at: include/llvm/Support/FormatProviders.h:37
+struct use_char_formatter
+    : public std::integral_constant<bool, std::is_same<T, char>::value> {};
+
----------------
inglorion wrote:
> Should this support unsigned char, too? What about wchar_t?
`unsigned char` is basically `uint8_t`, which is explicitly listed in the integral formatter.  An unsigned char usually doesn't denote ascii text, but rather just a raw byte.  So it makes sense to treat it the same as other numbers, while signed char is treated as ASCII.  Note that the char formatter's implementation delegates to the integer formatter (hence supporting all the same options as the integer one) if the style is anything other than empty.  So they're already very similar.


================
Comment at: include/llvm/Support/FormatProviders.h:87
+///
+///   ==========================================================================
+///   |  style  |     Meaning          |      Example     | Precision Meaning  |
----------------
inglorion wrote:
> 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.
No, but at least for hex, the difference between `0xABC` and `0xabc` is likely to start a heated debate :)  If lots of people feel strongly I can standardize on only one, but in my experience there are times when lowercase looks awkward and times when uppercase looks awkward, so I wouldn't want to force one or the other.


================
Comment at: lib/Support/NativeFormatting.cpp:72
+    double D = static_cast<double>(N);
+    if (IsNegative)
+      D = -D;
----------------
inglorion wrote:
> I don't follow - why do we have to check IsNegative when T is an unsigned type?
Because we can get here via `write_signed`.  Basically, this means "was the **original** number less than 0".  In `write_signed`, we check if it's less than 0 (hence meaning a negative has to be printed), then massage it to get the absolute value into an unsigned integer, then pass it into this function with `IsNegative=true`.  This allows us to have one function that prints any number, and if `IsNegative` is true it just puts a `-` in front.  Otherwise you end up with 3 or 4 functions which all look exactly the same except for 1-2 lines of code, which is irksome.


https://reviews.llvm.org/D25587