[libcxx-commits] [PATCH] D121138: [libc++][format] Adds formatter<charT[N], charT>.

[Mark de Wever via Phabricator via libcxx-commits]

Mordante marked an inline comment as done.
Mordante added inline comments.


================
Comment at: libcxx/include/__format/formatter_string.h:120
+};
+
 // Formatter const char[].
----------------
Quuxplusone wrote:
> > This formatter isn't in the list of required formatters in https://eel.is/c++draft/format.formatter.spec#2.2 [...]
> >
> > For each charT, the string type specializations
> > `template<> struct formatter<charT*, charT>;`
> > `template<> struct formatter<const charT*, charT>;`
> > `template<size_t N> struct formatter<const charT[N], charT>;`
> 
> This strikes me as a minor defect in the standard wording; @vitaut what do you think? Should the list of required formatters in https://eel.is/c++draft/format.formatter.spec#2.2 simply be updated to mention
> 
> > `template<size_t N> struct formatter<charT[N], charT>;`
> 
> and then leave
> 
> > `template<size_t N> struct formatter<const charT[N], charT>;`
> 
> to be covered automatically by... um... wait... does `formatter`'s specification //not// give any partial specializations to deal with cvref-qualified types? Is that a problem? Or is it actually OK if `formatter<const char[N]>`, `formatter<const int&>`, etc, fail to exist, because the STL code will never try to use them anyway? It looks like the STL code only ever tries to use `formatter<remove_cvref_t<T>>`. I don't think the exact formatting algorithm is specified anywhere in the standard, although https://eel.is/c++draft/format#functions-20 gives a good hint as to the intended implementation.
> 
> Anyway, I would:
> - treat this as a bug in the standard
> - file an LWG issue about it
> - just eliminate the bogus `const` on old line 113/new line 124, instead of adding a whole new partial specialization
The formatter is indeed not used, but without it a string literal isn't formattable. Which is an issue for the next patch in this series.

I had a private conversation with Victor and Charlie about this a while back. Removing `const char[N]` is an ABI break and an issue for MSVC STL. So removing it will cause implementation divergence. Victor thought it wasn't needed to file an LWG issue. (I'm somewhat on the fence about filing it.)


================
Comment at: libcxx/include/__format/formatter_string.h:128
 
   _LIBCPP_HIDE_FROM_ABI auto format(const _CharT __str[_Size], auto& __ctx)
       -> decltype(__ctx.out()) {
----------------
Quuxplusone wrote:
> Pre-existing, but please fix this at the same time: https://quuxplusone.github.io/blog/2020/03/12/contra-array-parameters/
> This should be
> ```
> _LIBCPP_HIDE_FROM_ABI auto format(const _CharT *__str, auto& __ctx)
> ```
> (Note, even when the `const` on line 113/124 is removed, the `const` on line 117/128 will remain, because we don't intend to modify the pointee.)
In this case I prefer to keep it as is. There's a separate formatter for `const char*` and `char*`. So here using the array shows it's a different function. Also the size is correct, when it's wrong there's UB since it's used in the constructor of the `basic_string_view`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121138