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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante accepted this revision.
Mordante marked 4 inline comments as done.
Mordante added inline comments.


================
Comment at: libcxx/include/__format/formatter_string.h:120
+};
+
 // Formatter const char[].
----------------
Mordante wrote:
> vitaut wrote:
> > Quuxplusone wrote:
> > > Mordante wrote:
> > > > 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.)
> > > @vitaut @barcharcraz , I think we should file an LWG issue about this. There's no point in the standard specifying a type that literally can't be used, while meanwhile //failing// to specify a type that is actually necessary for the entire machine to work correctly. Institutional knowledge is great, but if the standard means `formatter<char[N]>`, it should actually //say// that; it shouldn't leave it up to a consensus of the vendors to understand that "well, that one `const` is just kidding."
> > > 
> > > This has also made me much more confident that we should remove the specialization of `formatter<const char[N]>` at the same time, so that people don't start relying on it. (This is the same line of argument that leads me to want us to const-qualify our format functions even though that LWG issue is still open.)
> > > I think we should file an LWG issue about this.
> > 
> > Please do. I agree with your analysis that `const` shouldn't be there which follows from https://eel.is/c++draft/format#functions-20.
> I'll file an LWG issue.
I've filed an LWG issue yesterday.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121138