[libcxx-commits] [PATCH] D125606: [libc++][format] Improve string formatters

Mark de Wever via Phabricator via libcxx-commits libcxx-commits at lists.llvm.org
Tue Jun 21 09:00:01 PDT 2022

Mordante marked 7 inline comments as done.
Mordante added a comment.

Thanks for the reviews!

Comment at: libcxx/include/__format/parser_std_format_spec.h:1515
+/// must be ABI stable. To aid the stability the unused bits in the class are
+/// set to zero. That way they can be repurposed when a future revision of the
+/// Standards adds new fields to std-format-spec.
ldionne wrote:
Fair point. Looking at the amount of papers aiming to use format I still guess it's a matter of when ;-)

Comment at: libcxx/include/__format/parser_std_format_spec.h:1750-1751
+    __width_ = __format_spec::__substitute_arg_id(__arg);
+    if (__width_ == 0)
+      __throw_format_error("A format-spec width field replacement should have a positive value");
+  }
vitaut wrote:
> Mordante wrote:
> > vitaut wrote:
> > > This is a weird check. I think we should open an LWG issue to allow zero dynamic width and remove this unnecessary check.
> > Currently width is specified as
> > ```
> > width:
> >     positive-integer
> >     { arg-idopt }
> > ```
> > 
> > do you want to change that also to `nonnegative-integer`? When not it's the question what the width of `0` should do.
> > Note that `__format_spec::__substitute_arg_id(__arg);` already throws on negative values.
> > 
> > Another option would be to give the minimum as an argument to `__substitute_arg_id` so there it can validate the proper boundary.
> The grammar was done that way to avoid confusion between zero width and zero flag. I am not suggesting to change the grammar, just remove the runtime check for zero dynamic width since it's mostly harmless and only adds overhead.
I've filed an LWG-issue Sunday, based on previous experience it will be processed next weekend.
(I don't feel the need to add the issue in the comments of the code.)

  rG LLVM Github Monorepo



More information about the libcxx-commits mailing list