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

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone added subscribers: vitaut, Quuxplusone.
Quuxplusone requested changes to this revision.
Quuxplusone added inline comments.
This revision now requires changes to proceed.


================
Comment at: libcxx/include/__format/formatter_string.h:120
+};
+
 // Formatter const char[].
----------------
> 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


================
Comment at: libcxx/include/__format/formatter_string.h:128
 
   _LIBCPP_HIDE_FROM_ABI auto format(const _CharT __str[_Size], auto& __ctx)
       -> decltype(__ctx.out()) {
----------------
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.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121138