[libc++][format] Adds formatter for tuple and pair

[Mordante]

Summary

Implements parts of

• P2286R8 Formatting Ranges

[vitaut]

Overall looks good but we need to fix escaping in the standard and the implementation before this ships. Also a bunch of minor(ish) comments inline.

libcxx/include/__format/formatter.h

45–46

Note that there is a paper in flight to change set_debug_format API. Do you see any problems with applying the change if the current diff lands?

libcxx/include/__format/formatter_tuple.h

64

nit: tuple-format-spec sounds too standardese and it's not super clear what elements the error refers to, I would suggest something along the lines of "The format specifier m requires a two-element tuple".

73–75

This won't be called in case of an early return above.

78

Similarly, I suggest making this error message a bit more user-friendly e.g. "Unknown format specifier for tuple". Right now it is a bit too standardese (format-spec) and describing the implementation rather than the observable problem.

85

Do you know why we do this conditional_t dance? For tuples with move-only elements?

92

This function looks unnecessary, I suggest merging into format.

120

Maybe store the output iterator in a local variable and only call advance_to when you need to pass the context externally?

136

Why do you need to specify alignment here?

139

I don't think you need this because the formatter is only used in the format function and can be constructed there. This will be more efficient and simpler.

libcxx/include/__format/parser_std_format_spec.h

111–118

Not related to this diff but __type_ should probably be renamed to something more descriptive.

117

Maybe __allow_colon_in_fill_ would be more descriptive?

390

Again, I would recommend avoiding standardese terms. Maybe something like "The fill format specifier contains an invalid character"?

libcxx/test/std/utilities/format/format.tuple/format.verify.cpp

22 ↗

(On Diff #471734)

Out of curiosity, what does @*:* syntax mean here?

libcxx/test/std/utilities/format/format.tuple/format_tests.h

58 ↗

(On Diff #471734)

Add a test case with a multibyte fill?

157 ↗

(On Diff #471734)

Add a test case with invalid UTF-8?

302–303 ↗

(On Diff #471734)

We should fix this.

libcxx/test/std/utilities/format/format.tuple/pair.pass.cpp

34 ↗

(On Diff #471734)

nit: {} is redundant

[Mordante]

Thanks for the review!

In D136775#3968626, @vitaut wrote:

Overall looks good but we need to fix escaping in the standard and the implementation before this ships. Also a bunch of minor(ish) comments inline.

I too think we should fix escaping. I also think we should fix the nesting of formatters.
Normally we only implement papers after they have been voted in. That is a bit problematic since the next LLVM version will
branch a few weeks before the Issaquah meeting. I would like to see how the papers are received in LEWG, when positive I'll discuss with @ldionne which direction we want to take. (@ldionne I've seen the papers for both issues and I expect they will be in the next WG21 mailing.)

libcxx/include/__format/formatter.h

45–46

That won't be an issue. I've implemented your paper P2733R0 locally, on top of a work-in-progress range-formatting branch.

libcxx/include/__format/formatter_tuple.h

73–75

Nice catch. I didn't notice since that doesn't happen in practice since that only happens when the format string has no terminal '}'. So this version passes all tests.

I have moved it to be Standard conforming.

78

I use this message at other places too. Improving all error messages is still on my todo list.

85

I haven't checked, why. But I too assume it's for move-only elements.

120

While iterating over the underlying formatters we call advance every iteration. So I don't feel using a local variable helps a lot.

136

This is the default in the paper, therefore I set it explicitly.

139

I followed your suggestion, but unfortunately __underlying_ is used in parse. It could be solved by calling set_debug_format in format. However that gives a change of behaviour; nested formatters will enable debug formatting. I've added a comment instead. This suggestion can be applied when implementing P2733R0.

(I prefer to keep the underlying_ in the Standard since that makes it easier when we enable parsing for the underlying formatters.)

libcxx/include/__format/parser_std_format_spec.h

111–118

The names here match the names of the fields in the Standard. In that context I feel the name __type_ is descriptive.

117

I like that suggestion! I've updated the comments too.

390

I will do that in a follow-up. Then I use the same message for both the "normal" and the "range" fill character.

libcxx/test/std/utilities/format/format.tuple/format.verify.cpp

22 ↗

(On Diff #471734)

This is a way to say "I don't care where the diagnostic is generated". This error checking is heavily used in Clang and LLVM where the message usually generated on the next line in the test. This error is generated in one of the format headers, using the exact location is a maintenance burden. Adding or removing a line it the header or refactoring headers will break this test. So in libc++ we use this style to reduce the maintenance in libc++. (We still run into test failures when the compiler changes its diagnostic, but that doesn't happen often.)

libcxx/test/std/utilities/format/format.tuple/format_tests.h

58 ↗

(On Diff #471734)

libc++ doesn't support multibyte fill for char.

I haven't implemented, not yet voted in paper, P2572 std::format() fill character allowances yet.

157 ↗

(On Diff #471734)

I left thous out on purpose. Invalid Unicode sequences depend: on the type char/wchar_t, for wchar_t its size, and whether users have selected a library without Unicode support. These are tested in other places. Here I just test the basics of the escaping.

302–303 ↗

(On Diff #471734)

Agreed, I've added a comment regarding P2733.

[Mordante]

Addresses review comments.

[Mordante]

CI fixes.

[Mordante]

CI fixes.

[Mordante]

CI fixes.

[Mordante]

libcxx/test/std/utilities/format/format.tuple/pair.pass.cpp

34 ↗

(On Diff #471734)

I removed it, but it turns out not to be redundant. When omitted GCC will issue a diagnostic. I then recalled that was why I added it before, so added a comment.

[ldionne]

Mostly LGTM but I would like to see again for for_each.

libcxx/docs/Status/FormatPaper.csv

28

I too think we should fix escaping. I also think we should fix the nesting of formatters.
Normally we only implement papers after they have been voted in. That is a bit problematic since the next LLVM version
will branch a few weeks before the Issaquah meeting. I would like to see how the papers are received in LEWG, when
positive I'll discuss with @ldionne which direction we want to take.

If these issues were already in the pipeline and we knew pretty much which way things were going to go, I would say "sure, let's implement the expected issue resolution right away". However, it seems that we're a bit earlier than this here. I would recommend implementing whatever the standard says right now and have another patch ready to apply the expected issue resolution once it is discussed and we know which way we'll go.

We can also raise the unfortunate timing of the committee meeting and the LLVM release with the release manager -- perhaps it would be better for everyone to push back the LLVM branch point by 2 weeks or so.

libcxx/include/__chrono/statically_widen.h

36

Stray whitespace?

libcxx/include/__format/formatter_tuple.h

58–59

Let's use __for_each here too.

119

Here I would really suggest doing this to simplify things a bit:

auto __for_each = []<size_t ..._Index>(index_sequence<_Index...>, auto __func) {
  (__func.operator()<_Index>(), ...);
};

__for_each(make_index_sequence<sizeof...(_Args)>(), [&]<size_t _Index>() {
  // current code
});

This gets rid of one level of nesting. Also this __for_each function here should be extracted into a more general utility, since this has nothing specific to format.

Also it's unusual to use double-underscore with capital letter, so I'd stick to something more representative like _Index. And once you factor out __for_each, you don't need to number your indices anymore.

125–133

Can you add a TODO(format) here and also above where we currently set the debug format? IIUC the above one needs to be removed when we use this #if 0 branch.

libcxx/include/__format/parser_std_format_spec.h

120

TODO FMT We're now at 7 fields, should we increase the size for ABI reasons?

I think instead we should find a way to make sure that this type doesn't need to be kept ABI-stable. I think we can probably achieve this by using an ABI tag on it, since this is never stored in a struct.

libcxx/test/std/utilities/format/format.tuple/format.pass.cpp

8

Here and elsewhere, can you please add a synopsis of what you're testing?

[Mordante]

Thanks for the review!

libcxx/include/__chrono/statically_widen.h

36

No clang-format.

libcxx/include/__format/formatter_tuple.h

119

Looking at the code afterwards this is indeed an improvement, also the one above without the fold expression looks better.
Thanks for the suggestion.

libcxx/include/__format/parser_std_format_spec.h

120

I've given it some more thought and I indeed think this is ABI save since it's only used for function arguments and these all use an ABI tag.

libcxx/test/std/utilities/format/format.tuple/format.pass.cpp

8

Based on our discussion regarding D139561 I did a bit of renaming of these tests to make sure it's clear this tests the format function std::format and not the formatter's member function format.

[Mordante]

Rebased, address review comments, and improve tests.

[Mordante]

CI fixes.

[Mordante]

CI fixes.

[ldionne]

libcxx/include/__chrono/statically_widen.h

48

libcxx/include/__format/formatter.h

49–50

libcxx/include/__format/formatter_tuple.h

60

std::? Just for consistency.

libcxx/include/__format/parser_std_format_spec.h

105

So then here we would also need to ABI-tag this. What we want is like _LIBCPP_HIDE_FROM_ABI but for types. I think it would do:

// like _LIBCPP_HIDE_FROM_ABI but without the __exclude_from_explicit_instantiation__
#define _LIBCPP_HIDE_TYPE_FROM_ABI _LIBCPP_HIDDEN __attribute__((__abi_tag__(_LIBCPP_TOSTRING(_LIBCPP_VERSIONED_IDENTIFIER))))

Can you add a TODO instead?

libcxx/include/__utility/integer_sequence.h

141–144

I would avoid using an inline variable here. They have the unfortunate side effect of creating a weak def for the variable itself, which is not really useful here. Instead, I would just use a normal function template. It'll also be more consistent.

[Mordante]

Rebased and addresses review comments. Give a spin before landing.

[Mordante]

Thanks for the reviews!

[Mordante]

CI fixes.