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

[Zachary Turner via llvm-commits]

zturner added inline comments.


================
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);
----------------
amccarth wrote:
> 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.
Yea, I can make that change.


================
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());
----------------
amccarth wrote:
> 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.
LLVM already assumes US-centric localization for everything else, so this being consistent just seems like the right thing to do.  I don't know of any properly localized compilers :)


================
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)) {
----------------
amccarth wrote:
> Why?  Is this important for peformance or portability or something else?
Performance.  FWIW, the original code for this was in `raw_ostream` and this patch moves it over here.  But the original code did the same thing.  Anyway, the TL;DR is: If a number can fit in 32-bits, it's faster to use 32-bit division and mod than 64-bit division and mod.


================
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));
+
----------------
amccarth wrote:
> What's supposed to happen if you try to percent-format an integer other than 0 or 1?
Whatever integer you give it is multipled by 100 and printed.


Repository:
  rL LLVM

https://reviews.llvm.org/D25497