Refactor precision parsing

include/fmt/chrono.h

@@ -2000,36 +2000,6 @@ struct formatter<std::chrono::duration<Rep, Period>, Char> {
   basic_string_view<Char> format_str;
   using duration = std::chrono::duration<Rep, Period>;
 
-  struct spec_handler {
-    formatter& f;
-    basic_format_parse_context<Char>& context;
-    basic_string_view<Char> format_str;
-
-    template <typename Id> FMT_CONSTEXPR arg_ref_type make_arg_ref(Id arg_id) {
-      context.check_arg_id(arg_id);
-      return arg_ref_type(arg_id);
-    }
-
-    FMT_CONSTEXPR arg_ref_type make_arg_ref(basic_string_view<Char> arg_id) {
-      context.check_arg_id(arg_id);
-      return arg_ref_type(arg_id);
-    }
-
-    FMT_CONSTEXPR arg_ref_type make_arg_ref(detail::auto_id) {
-      return arg_ref_type(context.next_arg_id());
-    }
-
-    void on_error(const char* msg) { FMT_THROW(format_error(msg)); }
-    FMT_CONSTEXPR void on_precision(int _precision) {
-      f.precision = _precision;
-    }
-    FMT_CONSTEXPR void end_precision() {}
-
-    template <typename Id> FMT_CONSTEXPR void on_dynamic_precision(Id arg_id) {
-      f.precision_ref = make_arg_ref(arg_id);
-    }
-  };
-
   using iterator = typename basic_format_parse_context<Char>::iterator;
   struct parse_range {
     iterator begin;
@@ -2039,7 +2009,6 @@ struct formatter<std::chrono::duration<Rep, Period>, Char> {
   FMT_CONSTEXPR parse_range do_parse(basic_format_parse_context<Char>& ctx) {
     auto begin = ctx.begin(), end = ctx.end();
     if (begin == end || *begin == '}') return {begin, begin};
-    auto handler = spec_handler{*this, ctx, format_str};
 
     auto align_result = detail::parse_align(begin, end);
     specs.align = align_result.align;
@@ -2048,28 +2017,41 @@ struct formatter<std::chrono::duration<Rep, Period>, Char> {
     begin = align_result.end;
     if (begin == end) return {begin, begin};
 
-    auto width_result = detail::parse_width(begin, end, ctx);
+    auto width = detail::parse_dynamic_spec(begin, end, ctx);
-    auto width = width_result.width;
+    switch (width.spec.kind) {
-    switch (width.kind) {
     case detail::dynamic_spec_kind::none:
       break;
     case detail::dynamic_spec_kind::value:
-      specs.width = width.value;
+      specs.width = width.spec.value;
       break;
     case detail::dynamic_spec_kind::index:
-      width_ref = arg_ref_type(width.value);
+      width_ref = arg_ref_type(width.spec.value);
       break;
     case detail::dynamic_spec_kind::name:
-      width_ref = arg_ref_type(width.name);
+      width_ref = arg_ref_type(width.spec.name);
       break;
     }
-    begin = width_result.end;
+    begin = width.end;
     if (begin == end) return {begin, begin};
 
     auto checker = detail::chrono_format_checker();
     if (*begin == '.') {
       checker.has_precision_integral = !std::is_floating_point<Rep>::value;
-      begin = detail::parse_precision(begin, end, handler);
+      auto prec = detail::parse_precision(begin, end, ctx);
+      switch (prec.spec.kind) {
+      case detail::dynamic_spec_kind::none:
+        break;
+      case detail::dynamic_spec_kind::value:
+        precision = prec.spec.value;
+        break;
+      case detail::dynamic_spec_kind::index:
+        precision_ref = arg_ref_type(prec.spec.value);
+        break;
+      case detail::dynamic_spec_kind::name:
+        precision_ref = arg_ref_type(prec.spec.name);
+        break;
+      }
+      begin = prec.end;
     }
     if (begin != end && *begin == 'L') {
       ++begin;

include/fmt/core.h

@@ -2243,9 +2243,6 @@ template <typename Char> class specs_setter {
     specs_.fill[0] = Char('0');
   }
 
-  FMT_CONSTEXPR void on_precision(int precision) {
-    specs_.precision = precision;
-  }
   FMT_CONSTEXPR void end_precision() {}
 
   FMT_CONSTEXPR void on_type(presentation_type type) { specs_.type = type; }
@@ -2265,24 +2262,36 @@ class dynamic_specs_handler
 
   FMT_CONSTEXPR auto parse_context() -> ParseContext& { return context_; }
 
-  FMT_CONSTEXPR void on_width(const dynamic_spec<char_type>& width) {
+  FMT_CONSTEXPR void on_width(const dynamic_spec<char_type>& spec) {
-    switch (width.kind) {
+    switch (spec.kind) {
     case dynamic_spec_kind::none:
       break;
     case dynamic_spec_kind::value:
-      specs_.width = width.value;
+      specs_.width = spec.value;
       break;
     case dynamic_spec_kind::index:
-      specs_.width_ref = arg_ref_type(width.value);
+      specs_.width_ref = arg_ref<char_type>(spec.value);
       break;
     case dynamic_spec_kind::name:
-      specs_.width_ref = arg_ref_type(width.name);
+      specs_.width_ref = arg_ref<char_type>(spec.name);
       break;
     }
   }
 
-  template <typename Id> FMT_CONSTEXPR void on_dynamic_precision(Id arg_id) {
+  FMT_CONSTEXPR void on_precision(const dynamic_spec<char_type>& spec) {
-    specs_.precision_ref = make_arg_ref(arg_id);
+    switch (spec.kind) {
+    case dynamic_spec_kind::none:
+      break;
+    case dynamic_spec_kind::value:
+      specs_.precision = spec.value;
+      break;
+    case dynamic_spec_kind::index:
+      specs_.precision_ref = arg_ref<char_type>(spec.value);
+      break;
+    case dynamic_spec_kind::name:
+      specs_.precision_ref = arg_ref<char_type>(spec.name);
+      break;
+    }
   }
 
   FMT_CONSTEXPR void on_error(const char* message) {
@@ -2292,26 +2301,6 @@ class dynamic_specs_handler
  private:
   dynamic_format_specs<char_type>& specs_;
   ParseContext& context_;
-
-  using arg_ref_type = arg_ref<char_type>;
-
-  FMT_CONSTEXPR auto make_arg_ref(int arg_id) -> arg_ref_type {
-    context_.check_arg_id(arg_id);
-    context_.check_dynamic_spec(arg_id);
-    return arg_ref_type(arg_id);
-  }
-
-  FMT_CONSTEXPR auto make_arg_ref(auto_id) -> arg_ref_type {
-    int arg_id = context_.next_arg_id();
-    context_.check_dynamic_spec(arg_id);
-    return arg_ref_type(arg_id);
-  }
-
-  FMT_CONSTEXPR auto make_arg_ref(basic_string_view<char_type> arg_id)
-      -> arg_ref_type {
-    context_.check_arg_id(arg_id);
-    return arg_ref_type(arg_id);
-  }
 };
 
 template <typename Char> constexpr bool is_ascii_letter(Char c) {
@@ -2475,40 +2464,41 @@ FMT_CONSTEXPR FMT_INLINE auto parse_arg_id(const Char* begin, const Char* end,
   return begin;
 }
 
-template <typename Char> struct parse_width_result {
+template <typename Char> struct dynamic_spec_id_handler {
-  const Char* end;
+  basic_format_parse_context<Char>& ctx;
-  dynamic_spec<Char> width;
+  dynamic_spec<Char> spec;
+
+  FMT_CONSTEXPR void operator()() {
+    spec.kind = dynamic_spec_kind::index;
+    spec.value = ctx.next_arg_id();
+    ctx.check_dynamic_spec(spec.value);
+  }
+  FMT_CONSTEXPR void operator()(int id) {
+    spec.kind = dynamic_spec_kind::index;
+    spec.value = id;
+    ctx.check_arg_id(id);
+    ctx.check_dynamic_spec(id);
+  }
+  FMT_CONSTEXPR void operator()(basic_string_view<Char> id) {
+    spec.kind = dynamic_spec_kind::name;
+    spec.name = id;
+    ctx.check_arg_id(id);
+  }
+  FMT_CONSTEXPR void on_error(const char* message) {
+    if (message) throw_format_error("invalid format string");
+  }
 };
 
+template <typename Char> struct parse_dynamic_spec_result {
+  const Char* end;
+  dynamic_spec<Char> spec;
+};
+
+// Parses [integer | "{" [arg_id] "}"].
 template <typename Char>
-FMT_CONSTEXPR auto parse_width(const Char* begin, const Char* end,
+FMT_CONSTEXPR auto parse_dynamic_spec(const Char* begin, const Char* end,
-                               basic_format_parse_context<Char>& ctx)
+                                      basic_format_parse_context<Char>& ctx)
-    -> parse_width_result<Char> {
+    -> parse_dynamic_spec_result<Char> {
-  struct id_handler {
-    basic_format_parse_context<Char>& ctx;
-    dynamic_spec<Char> spec;
-
-    FMT_CONSTEXPR void operator()() {
-      spec.kind = dynamic_spec_kind::index;
-      spec.value = ctx.next_arg_id();
-      ctx.check_dynamic_spec(spec.value);
-    }
-    FMT_CONSTEXPR void operator()(int id) {
-      spec.kind = dynamic_spec_kind::index;
-      spec.value = id;
-      ctx.check_arg_id(id);
-      ctx.check_dynamic_spec(id);
-    }
-    FMT_CONSTEXPR void operator()(basic_string_view<Char> id) {
-      spec.kind = dynamic_spec_kind::name;
-      spec.name = id;
-      ctx.check_arg_id(id);
-    }
-    FMT_CONSTEXPR void on_error(const char* message) {
-      if (message) throw_format_error("invalid format string");
-    }
-  };
-
   FMT_ASSERT(begin != end, "");
   if ('0' <= *begin && *begin <= '9') {
     int value = parse_nonnegative_int(begin, end, -1);
@@ -2516,7 +2506,8 @@ FMT_CONSTEXPR auto parse_width(const Char* begin, const Char* end,
     throw_format_error("number is too big");
   } else if (*begin == '{') {
     ++begin;
-    auto handler = id_handler{ctx, {dynamic_spec_kind::none, {}}};
+    auto handler =
+        dynamic_spec_id_handler<Char>{ctx, {dynamic_spec_kind::none, {}}};
     if (begin != end) begin = parse_arg_id(begin, end, handler);
     if (begin != end && *begin == '}') {
       ++begin;
@@ -2527,42 +2518,16 @@ FMT_CONSTEXPR auto parse_width(const Char* begin, const Char* end,
   return {begin, {dynamic_spec_kind::none, {}}};
 }
 
-template <typename Char, typename Handler>
+template <typename Char>
 FMT_CONSTEXPR auto parse_precision(const Char* begin, const Char* end,
-                                   Handler&& handler) -> const Char* {
+                                   basic_format_parse_context<Char>& ctx)
-  using detail::auto_id;
+    -> parse_dynamic_spec_result<Char> {
-  struct precision_adapter {
-    Handler& handler;
-
-    FMT_CONSTEXPR void operator()() { handler.on_dynamic_precision(auto_id()); }
-    FMT_CONSTEXPR void operator()(int id) { handler.on_dynamic_precision(id); }
-    FMT_CONSTEXPR void operator()(basic_string_view<Char> id) {
-      handler.on_dynamic_precision(id);
-    }
-    FMT_CONSTEXPR void on_error(const char* message) {
-      if (message) handler.on_error(message);
-    }
-  };
-
   ++begin;
-  auto c = begin != end ? *begin : Char();
+  if (begin == end || *begin == '}') {
-  if ('0' <= c && c <= '9') {
+    throw_format_error("missing precision");
-    auto precision = parse_nonnegative_int(begin, end, -1);
+    return {begin, {dynamic_spec_kind::none, {}}};
-    if (precision != -1)
-      handler.on_precision(precision);
-    else
-      handler.on_error("number is too big");
-  } else if (c == '{') {
-    ++begin;
-    if (begin != end)
-      begin = parse_arg_id(begin, end, precision_adapter{handler});
-    if (begin == end || *begin++ != '}')
-      return handler.on_error("invalid format string"), begin;
-  } else {
-    return handler.on_error("missing precision specifier"), begin;
   }
-  handler.end_precision();
+  return parse_dynamic_spec(begin, end, ctx);
-  return begin;
 }
 
 template <typename Char>
@@ -2666,14 +2631,17 @@ FMT_CONSTEXPR FMT_INLINE auto parse_format_specs(const Char* begin,
     if (++begin == end) return begin;
   }
 
-  auto width_result = parse_width(begin, end, handler.parse_context());
+  auto width = parse_dynamic_spec(begin, end, handler.parse_context());
-  handler.on_width(width_result.width);
+  handler.on_width(width.spec);
-  begin = width_result.end;
+  begin = width.end;
   if (begin == end) return begin;
 
   // Parse precision.
   if (*begin == '.') {
-    begin = parse_precision(begin, end, handler);
+    auto precision = parse_precision(begin, end, handler.parse_context());
+    handler.on_precision(precision.spec);
+    if (precision.spec.kind != dynamic_spec_kind::none) handler.end_precision();
+    begin = precision.end;
     if (begin == end) return begin;
   }
 

include/fmt/format.h

@@ -3627,28 +3627,42 @@ template <typename Char> class specs_handler : public specs_setter<Char> {
     return parse_context_;
   }
 
-  FMT_CONSTEXPR void on_width(const dynamic_spec<Char>& width) {
+  FMT_CONSTEXPR void on_width(const dynamic_spec<Char>& spec) {
     auto arg = format_arg();
-    switch (width.kind) {
+    switch (spec.kind) {
     case dynamic_spec_kind::none:
       return;
     case dynamic_spec_kind::value:
-      this->specs_.width = width.value;
+      this->specs_.width = spec.value;
       return;
     case dynamic_spec_kind::index:
-      arg = detail::get_arg(context_, width.value);
+      arg = detail::get_arg(context_, spec.value);
       break;
     case dynamic_spec_kind::name:
-      arg = detail::get_arg(context_, width.name);
+      arg = detail::get_arg(context_, spec.name);
       break;
     }
     this->specs_.width =
         get_dynamic_spec<width_checker>(arg, context_.error_handler());
   }
 
-  template <typename Id> FMT_CONSTEXPR void on_dynamic_precision(Id arg_id) {
+  FMT_CONSTEXPR void on_precision(const dynamic_spec<Char>& spec) {
-    this->specs_.precision = get_dynamic_spec<precision_checker>(
+    auto arg = format_arg();
-        get_arg(arg_id), context_.error_handler());
+    switch (spec.kind) {
+    case dynamic_spec_kind::none:
+      return;
+    case dynamic_spec_kind::value:
+      this->specs_.precision = spec.value;
+      return;
+    case dynamic_spec_kind::index:
+      arg = detail::get_arg(context_, spec.value);
+      break;
+    case dynamic_spec_kind::name:
+      arg = detail::get_arg(context_, spec.name);
+      break;
+    }
+    this->specs_.precision =
+        get_dynamic_spec<precision_checker>(arg, context_.error_handler());
   }
 
   void on_error(const char* message) { context_.on_error(message); }

test/core-test.cc

@@ -568,10 +568,20 @@ struct test_format_specs_handler {
     }
   }
 
-  constexpr void on_precision(int p) { precision = p; }
+  constexpr void on_precision(const fmt::detail::dynamic_spec<char>& spec) {
-  constexpr void on_dynamic_precision(fmt::detail::auto_id) {}
+    switch (spec.kind) {
-  constexpr void on_dynamic_precision(int index) { precision_ref = index; }
+    case fmt::detail::dynamic_spec_kind::none:
-  constexpr void on_dynamic_precision(string_view) {}
+      break;
+    case fmt::detail::dynamic_spec_kind::value:
+      precision = spec.value;
+      break;
+    case fmt::detail::dynamic_spec_kind::index:
+      precision_ref = spec.value;
+      break;
+    case fmt::detail::dynamic_spec_kind::name:
+      break;
+    }
+  }
 
   constexpr void end_precision() {}
   constexpr void on_type(fmt::presentation_type t) { type = t; }

test/format-test.cc

@@ -929,9 +929,9 @@ TEST(format_test, precision) {
                    "number is too big");
 
   EXPECT_THROW_MSG((void)fmt::format(runtime("{0:."), 0), format_error,
-                   "missing precision specifier");
+                   "missing precision");
   EXPECT_THROW_MSG((void)fmt::format(runtime("{0:.}"), 0), format_error,
-                   "missing precision specifier");
+                   "missing precision");
 
   EXPECT_THROW_MSG((void)fmt::format(runtime("{0:.2"), 0), format_error,
                    "precision not allowed for this argument type");