Allow use of "," format specifier as in Python

f68771a helpfully enabled the "n" typecode for locale-aware printing of
integers, but not for floats. This commit enables the use of "n" for
locale-aware printing of floats, as in Python, and additionally
introduces the "," format specifier, which can be applied to "d", "f",
and "g" (and their upper-case variants, when present) to force
locale-aware grouping.

"," is particularly useful for floats, as "n" will use exponential
notation for large floats, while ",f" never will.

Unlike in Python, there is no distinction between ",d" and "n" for
integers, or ",g" and "n" for floats. (In Python, "," will always output
a literal comma, while "n" is locale-aware.) In fmtlib, both "," and "n"
are locale-aware.

doc/syntax.rst

@@ -74,14 +74,14 @@ although some of the formatting options are only supported by the numeric types.
 The general form of a *standard format specifier* is:
 
 .. productionlist:: sf
-   format_spec: [[`fill`]`align`][`sign`]["#"]["0"][`width`]["." `precision`][`type`]
+   format_spec: [[`fill`]`align`][`sign`]["#"]["0"][`width`][","]["." `precision`][`type`]
    fill: <a character other than '{' or '}'>
    align: "<" | ">" | "=" | "^"
    sign: "+" | "-" | " "
    width: `integer` | "{" `arg_id` "}"
    precision: `integer` | "{" `arg_id` "}"
    type: `int_type` | "c" | "e" | "E" | "f" | "F" | "g" | "G" | "p" | "s"
-   int_type: "b" | "B" | "d" | "o" | "x" | "X"
+   int_type: "b" | "B" | "d" | "n" | "o" | "x" | "X"
 
 The *fill* character can be any character other than '{' or '}'.  The presence
 of a fill character is signaled by the character following it, which must be
@@ -144,11 +144,12 @@ decimal-point character appears in the result of these conversions
 only if a digit follows it. In addition, for ``'g'`` and ``'G'``
 conversions, trailing zeros are not removed from the result.
 
-.. ifconfig:: False
+The ``","`` option signals the use of a locale-aware thousands
-
+separator. In many locales, this may not be an actual `,` character.
-   The ``','`` option signals the use of a comma for a thousands separator.
+This option is supported for types ``'d'``, ``'f'``, ``'g'``, ``'F'``,
-   For a locale aware separator, use the ``'n'`` integer presentation type
+``'G'``, and none. Type ``'n'`` always implies the use of a local-aware
-   instead.
+thousands separator; it is an error to explicitly specify both ``","``
+and ``'n'``.
 
 *width* is a decimal integer defining the minimum field width.  If not
 specified, then the field width will be determined by the content.
@@ -263,6 +264,10 @@ The available presentation types for floating-point values are:
 |         | ``'E'`` if the number gets too large. The                |
 |         | representations of infinity and NaN are uppercased, too. |
 +---------+----------------------------------------------------------+
+| ``'n'`` | Number. This is the same as ``'g'``, except that it uses |
+|         | the current locale setting to insert the appropriate     |
+|         | number separator characters.                             |
++---------+----------------------------------------------------------+
 | none    | The same as ``'g'``.                                     |
 +---------+----------------------------------------------------------+
 

fmt/format.h

@@ -1558,7 +1558,8 @@ enum Alignment {
 // Flags.
 enum {
   SIGN_FLAG = 1, PLUS_FLAG = 2, MINUS_FLAG = 4, HASH_FLAG = 8,
-  CHAR_FLAG = 0x10  // Argument has char type - used in error reporting.
+  GROUP_THOUSANDS_FLAG = 0x10,
+  CHAR_FLAG = 0x20  // Argument has char type - used in error reporting.
 };
 
 // An empty format specifier.
@@ -2717,7 +2718,11 @@ void BasicWriter<Char>::write_int(T value, Spec spec) {
     prefix[0] = spec.flag(PLUS_FLAG) ? '+' : ' ';
     ++prefix_size;
   }
-  switch (spec.type()) {
+  char type = spec.type();
+  if (spec.flag(GROUP_THOUSANDS_FLAG)) {
+    type = 'n';
+  }
+  switch (type) {
   case 0: case 'd': {
     unsigned num_digits = internal::count_digits(abs_value);
     CharPtr p = prepare_int_buffer(num_digits, spec, prefix, prefix_size) + 1;
@@ -2799,7 +2804,7 @@ void BasicWriter<Char>::write_double(T value, const FormatSpec &spec) {
   char type = spec.type();
   bool upper = false;
   switch (type) {
-  case 0:
+  case 0: case 'n':
     type = 'g';
     break;
   case 'e': case 'f': case 'g': case 'a':
@@ -2868,13 +2873,15 @@ void BasicWriter<Char>::write_double(T value, const FormatSpec &spec) {
   }
 
   // Build format string.
-  enum { MAX_FORMAT_SIZE = 10}; // longest format: %#-*.*Lg
+  enum { MAX_FORMAT_SIZE = 11}; // longest format: %#'-*.*Lg
   Char format[MAX_FORMAT_SIZE];
   Char *format_ptr = format;
   *format_ptr++ = '%';
   unsigned width_for_sprintf = width;
   if (spec.flag(HASH_FLAG))
     *format_ptr++ = '#';
+  if (spec.flag(GROUP_THOUSANDS_FLAG) || spec.type() == 'n')
+    *format_ptr++ = '\'';
   if (spec.align() == ALIGN_CENTER) {
     width_for_sprintf = 0;
   } else {
@@ -3638,6 +3645,11 @@ const Char *BasicFormatter<Char, ArgFormatter>::format(
       spec.width_ = static_cast<int>(value);
     }
 
+    if (*s == ',') {
+      spec.flags_ |= GROUP_THOUSANDS_FLAG;
+      ++s;
+    }
+
     // Parse precision.
     if (*s == '.') {
       ++s;
@@ -3687,6 +3699,17 @@ const Char *BasicFormatter<Char, ArgFormatter>::format(
     // Parse type.
     if (*s != '}' && *s)
       spec.type_ = static_cast<char>(*s++);
+
+    if (spec.flag(GROUP_THOUSANDS_FLAG)) {
+      switch (spec.type()) {
+      case 0: case 'd': case 'f': case 'F': case 'g': case 'G':
+        break;
+      default:
+        FMT_THROW(FormatError(
+            fmt::format("format specifier ',' not allowed for type '{}'",
+            spec.type())));
+      }
+    }
   }
 
   if (*s++ != '}')

test/format-test.cc

@@ -960,6 +960,21 @@ TEST(FormatterTest, RuntimeWidth) {
   EXPECT_EQ("str         ", format("{0:{1}}", "str", 12));
 }
 
+TEST(FormatterTest, GroupThousandsFlag) {
+  EXPECT_THROW_MSG(format("{0:,x", 42),
+      FormatError, "format specifier ',' not allowed for type 'x'");
+  EXPECT_THROW_MSG(format("{0:,b", 42),
+      FormatError, "format specifier ',' not allowed for type 'b'");
+  EXPECT_THROW_MSG(format("{0:,o", 42),
+      FormatError, "format specifier ',' not allowed for type 'o'");
+  EXPECT_THROW_MSG(format("{0:,n", 42),
+      FormatError, "format specifier ',' not allowed for type 'n'");
+  EXPECT_THROW_MSG(format("{0:,n", 42.3),
+      FormatError, "format specifier ',' not allowed for type 'n'");
+  EXPECT_THROW_MSG(format("{0:,e", 42.3),
+      FormatError, "format specifier ',' not allowed for type 'e'");
+}
+
 TEST(FormatterTest, Precision) {
   char format_str[BUFFER_SIZE];
   safe_sprintf(format_str, "{0:.%u", UINT_MAX);
@@ -1106,7 +1121,7 @@ template <typename T>
 void check_unknown_types(
     const T &value, const char *types, const char *type_name) {
   char format_str[BUFFER_SIZE], message[BUFFER_SIZE];
-  const char *special = ".0123456789}";
+  const char *special = ",.0123456789}";
   for (int i = CHAR_MIN; i <= CHAR_MAX; ++i) {
     char c = static_cast<char>(i);
     if (std::strchr(types, c) || std::strchr(special, c) || !c) continue;
@@ -1229,10 +1244,13 @@ TEST(FormatterTest, FormatIntLocale) {
   lconv lc = {};
   char sep[] = "--";
   lc.thousands_sep = sep;
-  EXPECT_CALL(mock, localeconv()).Times(3).WillRepeatedly(testing::Return(&lc));
+  EXPECT_CALL(mock, localeconv()).Times(6).WillRepeatedly(testing::Return(&lc));
   EXPECT_EQ("123", format("{:n}", 123));
   EXPECT_EQ("1--234", format("{:n}", 1234));
   EXPECT_EQ("1--234--567", format("{:n}", 1234567));
+  EXPECT_EQ("123", format("{:,d}", 123));
+  EXPECT_EQ("1--234", format("{:,d}", 1234));
+  EXPECT_EQ("1--234--567", format("{:,d}", 1234567));
 }
 
 TEST(FormatterTest, FormatFloat) {
@@ -1240,7 +1258,7 @@ TEST(FormatterTest, FormatFloat) {
 }
 
 TEST(FormatterTest, FormatDouble) {
-  check_unknown_types(1.2, "eEfFgGaA", "double");
+  check_unknown_types(1.2, "eEfFgGaAn", "double");
   EXPECT_EQ("0", format("{0:}", 0.0));
   EXPECT_EQ("0.000000", format("{0:f}", 0.0));
   EXPECT_EQ("392.65", format("{0:}", 392.65));
@@ -1260,6 +1278,26 @@ TEST(FormatterTest, FormatDouble) {
   EXPECT_EQ(buffer, format("{:A}", -42.0));
 }
 
+TEST(FormatterTest, FormatDoubleLocale) {
+  // Grouping floating-point numbers uses snprintf internally. glibc's
+  // implementation of snprintf determines the current locale's
+  // thousands separator without calling localeconv, so we can't simply
+  // mock localeconv as in FormatIntLocale--we have to set an actual
+  // locale with a non-empty thousands separator using setlocale.
+  char* oldlocale = std::setlocale(LC_ALL, "");
+  char* sep = localeconv()->thousands_sep;
+  // Ensure this locale has a non-empty thousands separator.
+  EXPECT_GE(strlen(sep), 1);
+  char buffer[100];
+  safe_sprintf(buffer, "3%s141%s592.653589", sep, sep);
+  EXPECT_EQ(buffer, format("{:,f}", 3141592.653589));
+  safe_sprintf(buffer, "3%s141.65", sep);
+  EXPECT_EQ(buffer, format("{:n}", 3141.653589));
+  EXPECT_EQ(buffer, format("{:,g}", 3141.653589));
+  EXPECT_EQ(buffer, format("{:,}", 3141.653589));
+  std::setlocale(LC_ALL, oldlocale);
+}
+
 TEST(FormatterTest, FormatNaN) {
   double nan = std::numeric_limits<double>::quiet_NaN();
   EXPECT_EQ("nan", format("{}", nan));