[PATCH] D25497: [Format] Add more ways to format numbers

[Adrian McCarthy via llvm-commits]

amccarth added a comment.

A late drive-by.



================
Comment at: llvm/trunk/lib/Support/NativeFormatting.cpp:60
 template<typename T, std::size_t N>
 static int format_to_buffer(T Value, char (&Buffer)[N]) {
   char *EndPtr = std::end(Buffer);
----------------
Shouldn't this be named `formatToBuffer`?  Or, perhaps more specifically, `formatIntegralTypeToBuffer`.  Since it's a template function that apparently accepts any type for Value, someone might otherwise mistake it for a more general formatter.


================
Comment at: llvm/trunk/lib/Support/NativeFormatting.cpp:76
 
-  char NumberBuffer[20];
-  int Len = format_to_buffer(N, NumberBuffer);
-  int Pad = (MinWidth == 0) ? 0 : MinWidth - Len;
-  if (Pad > 0)
-    S.indent(Pad);
-  S.write(std::end(NumberBuffer) - Len, Len);
+static void writeWithCommas(raw_ostream &S, ArrayRef<char> Buffer) {
+  assert(!Buffer.empty());
----------------
How very U.S.-centric.  Some regions use different characters as separators, and a few don't even group digits in clusters other than three.


================
Comment at: llvm/trunk/lib/Support/NativeFormatting.cpp:133
+
+  // One for the decimal sign, one for each point of precision.
+  size_t DecimalChars = WriteDecimal ? 1 + Prec : 0;
----------------
"Decimal point" or "decimal separator" seems more idiomatic than "decimal sign."


================
Comment at: llvm/trunk/lib/Support/NativeFormatting.cpp:181
+                           bool IsNegative = false) {
+  // Output using 32-bit div/mod if possible.
+  if (N == static_cast<uint32_t>(N)) {
----------------
Why?  Is this important for peformance or portability or something else?


================
Comment at: llvm/trunk/unittests/Support/NativeFormatTests.cpp:148
+
+  // Hex formatting.  Default precision is 0.
+  // lower case, prefix.
----------------
What does "default precision is 0" mean in this context?  Do the hex styles allow you to set a precision to something like 2 or 4 so that it always emits a whole number of bytes?  If so, how about a test case for that?


================
Comment at: llvm/trunk/unittests/Support/NativeFormatTests.cpp:245
+  // it is and we have to live with it unless we fix all existing users of
+  // prefixed hex formatting.
+  EXPECT_EQ("0x000", format_number(0, IntegerStyle::HexLowerPrefix, 5));
----------------
Weird, I would have expected this to use width rather than precision.


================
Comment at: llvm/trunk/unittests/Support/NativeFormatTests.cpp:288
+
+  // Additional precision should padd with 0s.
+  EXPECT_EQ("-00010", format_number(-10, IntegerStyle::Integer, 5));
----------------
s/padd/pad/


================
Comment at: llvm/trunk/unittests/Support/NativeFormatTests.cpp:345
+  EXPECT_EQ("    100%", format_number(1, IntegerStyle::Percent, None, 8));
+  EXPECT_EQ(" 100.000%", format_number(1, IntegerStyle::Percent, 3, 9));
+
----------------
What's supposed to happen if you try to percent-format an integer other than 0 or 1?


================
Comment at: llvm/trunk/unittests/Support/NativeFormatTests.cpp:354
+  EXPECT_EQ("4.24%", format_number(.042379, FloatStyle::Percent, 2, 5));
+  EXPECT_EQ("4.238%", format_number(.042379, FloatStyle::Percent, 3, 5));
+  EXPECT_EQ("  0.424%", format_number(.0042379, FloatStyle::Percent, 3, 8));
----------------
How about something greater than 100% to ensure it doesn't chop digits to make it fit in the width?


Repository:
  rL LLVM

https://reviews.llvm.org/D25497