This is an archive of the discontinued LLVM Phabricator instance.

[libc++][format] Adds formatter<charT[N], charT>.
ClosedPublic

Authored by Mordante on Mar 7 2022, 10:20 AM.

Details

Summary

This formatter isn't in the list of required formatters in

[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>;
template<class traits, class Allocator>
  struct formatter<basic_string<charT, traits, Allocator>, charT>;
template<class traits>
  struct formatter<basic_string_view<charT, traits>, charT>;

Since remove_cvref_t<const charT[N]> is charT[N] the formatter is
required by

[format.functions]/25

Preconditions: formatter<remove_cvref_t<Ti>, charT> meets the
BasicFormatter requirements ([formatter.requirements]) for each Ti in
Args.

Depends on D120921

Diff Detail

Event Timeline

Mordante created this revision.Mar 7 2022, 10:20 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 7 2022, 10:20 AM
Mordante requested review of this revision.Mar 7 2022, 10:20 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 7 2022, 10:20 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Quuxplusone requested changes to this revision.Mar 7 2022, 12:30 PM
Quuxplusone added inline comments.
libcxx/include/__format/formatter_string.h
114

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
122

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

This revision now requires changes to proceed.Mar 7 2022, 12:30 PM
Mordante updated this revision to Diff 413862.Mar 8 2022, 10:18 AM

Retry CI since passes locally.

Mordante marked an inline comment as done.Mar 8 2022, 12:02 PM
Mordante added inline comments.
libcxx/include/__format/formatter_string.h
114

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

122

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.

libcxx/include/__format/formatter_string.h
122

Also the size is correct, when it's wrong there's UB

No, that's not true. See https://quuxplusone.github.io/blog/2020/03/12/contra-array-parameters/
In both C and C++, a function type of the form int(int[42]) is exactly, 100%, equivalent to int(int*). As Linus says, writing the pointer parameter using square-bracket syntax is misleading and can lead only to further mistakes.

Quuxplusone added inline comments.
libcxx/include/__format/formatter_string.h
114

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

vitaut added inline comments.Mar 9 2022, 11:50 AM
libcxx/include/__format/formatter_string.h
114

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.

Mordante updated this revision to Diff 416694.Mar 19 2022, 6:32 AM
Mordante marked an inline comment as done.

Rebased and removed libcpp-no-concepts from tests.

Mordante added inline comments.Mar 19 2022, 6:34 AM
libcxx/include/__format/formatter_string.h
114

I'll file an LWG issue.

vitaut added inline comments.Mar 27 2022, 7:41 AM
libcxx/include/__format/formatter_string.h
105–107

I suggest adding a static assert that _CharT is non-const.

Mordante updated this revision to Diff 418459.Mar 27 2022, 9:41 AM

Address review comments.

Mordante updated this revision to Diff 421708.Apr 9 2022, 3:26 AM

Rebased for next patches in series.

Mordante updated this revision to Diff 427649.May 6 2022, 8:39 AM
Mordante marked an inline comment as done.

Rebase to trigger CI

Mordante accepted this revision.May 18 2022, 11:09 AM
Mordante marked 4 inline comments as done.
Mordante added inline comments.
libcxx/include/__format/formatter_string.h
114

I've filed an LWG issue yesterday.

This revision was not accepted when it landed; it landed in state Needs Review.May 18 2022, 11:10 AM
This revision was automatically updated to reflect the committed changes.