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

[Mark de Wever via Phabricator via libcxx-commits]

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.)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D125606/new/

https://reviews.llvm.org/D125606