This is an archive of the discontinued LLVM Phabricator instance.

[libc++][format] Implement formatters.
ClosedPublic

Authored by Mordante on Feb 14 2021, 6:01 AM.

Details

Summary

This implements the initial version of the std::formatter class and its specializations. It also implements the following formatting functions:

  • format
  • vformat
  • format_to
  • vformat_to
  • format_to_n
  • formatted_size

All functions have a char and wchar_t version. Parsing the format-spec and
using the parsed format-spec hasn't been implemented. The code isn't optimized,
neither for speed, nor for size.

The goal is to have the rudimentary basics working, which can be used as a
basis to improve upon. The formatters used in this commit are simple stubs that
will be replaced by real formatters in later commits.

The formatters that are slated to be replaced in this patch series don't have
an availability macro to avoid merge conflicts.

Note the formatter for bool uses 0 and 1 instead of "false" and
"true". This will be fixed when the stub is replaced with a real
formatter.

Implements parts of:

  • P0645 Text Formatting

Completes:

  • LWG3539 format_to must not copy models of output_iterator<const charT&>

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Removed iterator_different_t work-around
Use proper constrain in
vformat_to (_CharT instead of char)
Polishing

Mordante edited the summary of this revision. (Show Details)Jun 5 2021, 4:07 AM
Mordante added a reviewer: vitaut.
Mordante edited the summary of this revision. (Show Details)Jun 9 2021, 8:54 AM
vitaut requested changes to this revision.Jun 13 2021, 8:22 AM

Overall looks good, some comments inline.

libcxx/include/__format/format_string.h
39

I would recommend using a struct with meaningful field names instead of a pair. In addition to making the code more readable it would remove dependency on utility.

68

Is it even possible to have 1 billion arguments to a function? I don't think so and therefore this should probably be an assert instead of an exception.

92–94

The limit of 999.999.999 is OK for an argument ID but is unnecessarily restrictive for width and precision. {fmt} supports any nonnegative int value and so do common printf implementations (almost, some of them don't handle INT_MAX, but they do handle values >= 1,000,000,000: https://godbolt.org/z/Esb9e6Wxh).

Having such limit will make migration from printf and other formatting facilities more difficult, please remove or make sure that it's only used for argument IDs.

123

I suggest changing "argument" to "character" to avoid confusion with formatting arguments and since individual characters are not really arguments, pointers to them are.

141

nit: format-spec is a syntactic construct (http://eel.is/c++draft/format#string.general-1) so I suggest not capitalizing it or rephrasing the error message altogether (e.g. "Invalid character in arg-id" if it's common to have exception messages capitalized).

libcxx/include/__format/formatter.h
35

"is not conform" -> "does not conform to"

libcxx/include/format
59

string_view should be replaced with format_string<Args...> per http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2021/p2216r3.html but I guess it can be done in a separate diff.

340

Why add this instead of implementing parse/format in formatter specialization directly?

607

typo: sequence

621

Looks like the meaning is negated here, this is exactly when we do have format-specifier.

634

Arguments should be passed by value since they are cheap to copy. Then decay_t will probably be unnecessary.

768

What does PoC stand for?

libcxx/test/std/utilities/format/format.functions/tests.inc
114

party -> partly throughout the diff

129–131

You probably know that but the default format is wrong. The output should be 42 without decimal point or trailing zeros.

This revision now requires changes to proceed.Jun 13 2021, 8:22 AM
vitaut added inline comments.Jun 13 2021, 8:45 AM
libcxx/include/__format/format_string.h
112–114

I don't think this error is correct. For example, you could have a format-spec consisting of a single width passed to formatter::parse function which should handle it without an error.

libcxx/test/std/utilities/format/format.formatter/format.formatter.spec/formatter.bool.pass.cpp
13–17 ↗(On Diff #349177)

I'm not sure how to parse this comment. Is something missing here?

65–66 ↗(On Diff #349177)

These should be "true" and "false", not "1" and "0".

libcxx/test/std/utilities/format/format.formatter/format.formatter.spec/formatter.c_string.pass.cpp
66 ↗(On Diff #349177)

Might be worth testing a string with embedded NUL to make sure that we format until the first NUL rather than the whole string.

Mordante planned changes to this revision.Jun 22 2021, 10:43 AM
Mordante marked 13 inline comments as done.

Thanks for the review! I see two major changes to be done:

  • Allowing the format-spec to accept the end-of-input as a valid termination.
  • Increment the maximum value for width and precision.

These changes will affect the patches depending on this patch.
Before updating this patch I first want to update all affected patches.

libcxx/include/__format/format_string.h
68

Good point.

92–94

I expected 999.999.999 to be large enough. I still think formatting this number of characters isn't realistic. But since printf supports it I agree it would be good to allow this number here. This change will affect more patches depending on this one.

112–114

I see I originally interpretted http://eel.is/c++draft/format#formatter.requirements-2.7 differently.

Parses format-spec ([format.string]) for type T in the range [pc.begin(), pc.end()) until the first unmatched character.
Throws format_­error unless the whole range is parsed or the unmatched character is }.

I thought throwing here was valid, but rereading it, I agree with your interpretation.

libcxx/include/format
59

Yes P2216 will be done later.

768

Proof-of-concept.

libcxx/test/std/utilities/format/format.formatter/format.formatter.spec/formatter.bool.pass.cpp
13–17 ↗(On Diff #349177)

This is part of the standard wording of the part tested.

65–66 ↗(On Diff #349177)

Correct, that will be done in D103670. It's part of the commit message that this is a temporary version.

libcxx/test/std/utilities/format/format.functions/tests.inc
129–131

Yes this should be fixed once I add proper floating-point support.

vitaut added inline comments.Jun 23 2021, 11:05 AM
libcxx/test/std/utilities/format/format.formatter/format.formatter.spec/formatter.bool.pass.cpp
13–17 ↗(On Diff #349177)

The confusing to me part is that "a specialization" appears twice. I suggest replacing a specialization template<> struct formatter<ArithmeticT, charT>; with template<> struct formatter<ArithmeticT, charT>;

libcxx/test/std/utilities/format/format.formatter/format.formatter.spec/formatter.bool.pass.cpp
13–17 ↗(On Diff #349177)

Not to mention, "Implemants" on line 16 definitely isn't standard wording, and line 17 seems to be starting a bulleted list with only one item in it. I think this comment is meant to refer to https://eel.is/c++draft/format.formatter.spec#2.3 and so it should say:

// [format.formatter.spec]:
// Each header that declares the template `formatter` provides the following enabled specializations:
// For each `charT`, for each cv-unqualified arithmetic type `ArithmeticT` other than
// char, wchar_­t, char8_­t, char16_­t, or char32_­t, a specialization
//    template<> struct formatter<ArithmeticT, charT>
//
// This file tests with `ArithmeticT=bool`, for each valid `charT`.
29–43 ↗(On Diff #349177)

I'd find this easier to read:

template <class StringT, class ArithmeticT>
void test(StringT expected, StringT fmt, ArithmeticT arg) {
  using CharT = typename StringT::value_type;
  auto parse_ctx = std::basic_format_parse_context<CharT>(fmt);
  std::formatter<bool, CharT> formatter;
  formatter.parse(parse_ctx);

  StringT result;
  auto out = std::back_inserter(result);
  using FormatCtxT = std::basic_format_context<decltype(out), CharT>;

  auto format_ctx = FormatCtxT(out, std::make_format_args<FormatCtxT>(arg));
  formatter.format(arg, format_ctx);
  assert(result == expected);
}

And then, if ArithmeticT is always bool, I strongly recommend making the substitution. Otherwise, you're implicitly relying on template type deduction down in main to deduce bool from true and false — which is obviously not-too-bad in this case, but you're going to have to use a different strategy once you reach the formatter for short. So I recommend switching strategies right now, in order to be consistent across all the new test files.

template <class StringT>
void test(StringT expected, StringT fmt, bool arg) {

(and then the rest as above)

59–62 ↗(On Diff #349177)

These asserts seem non-standard: I mean, I would by default assume there's no legal guarantee that &F::parse compiles at all. parse might be a template or overload set.
@vitaut , what's your understanding and/or intent here? Are user-programmers supposed to be able to take member pointers to the parse and format callables like this? (I hope not!)

vitaut added inline comments.Jun 23 2021, 4:17 PM
libcxx/test/std/utilities/format/format.formatter/format.formatter.spec/formatter.bool.pass.cpp
59–62 ↗(On Diff #349177)

IIRC users have no right to rely on specific signatures of functions in std so I would recommend deleting these over-specified asserts.

Mordante marked 8 inline comments as done.Jun 24 2021, 11:51 AM

I'm still working on the removal of the 999.999.999 in the underlaying patches. It causes quite a bit of changes.

libcxx/test/std/utilities/format/format.formatter/format.formatter.spec/formatter.bool.pass.cpp
13–17 ↗(On Diff #349177)

I overlooked the double specialization. I'll adjust the text.

59–62 ↗(On Diff #349177)

I'll remove them.

Mordante marked 10 inline comments as done.Jun 27 2021, 5:21 AM
Mordante added inline comments.
libcxx/include/format
340

Not entirely sure what you mean, but this stub class will be removed in D103466.

621

Good catch.

libcxx/test/std/utilities/format/format.formatter/format.formatter.spec/formatter.bool.pass.cpp
29–43 ↗(On Diff #349177)

I used most of this suggestion and updated the other tests as well. In this case I prefer to not add another layer of indirection, instead I replaced the template argument ArithmeticT with a bool.

libcxx/test/std/utilities/format/format.formatter/format.formatter.spec/formatter.c_string.pass.cpp
66 ↗(On Diff #349177)

I like this suggestion. I also added them to the other string types where the NUL is allowed.

Mordante updated this revision to Diff 354741.Jun 27 2021, 5:37 AM
Mordante marked 4 inline comments as done.

Addresses review comments:

  • Increment the allowed width and precision to INT32_MAX
  • Improve the unit tests
  • Remove usage of std::pair
  • Don't require the replacement field to end at a '}'
  • Several minor cleanups and fixes
Mordante updated this revision to Diff 354742.Jun 27 2021, 6:07 AM

Disable failing unit tests on GCC.

libcxx/test/std/utilities/format/format.formatter/format.formatter.spec/formatter.bool.pass.cpp
29–43 ↗(On Diff #349177)

LGTM! Btw, @vitaut, is this really the most ergonomic way to fit all these puzzle pieces together? I know this isn't the "expected way to use <format>," because the expected way is just std::format("{}", arg)... but this code feels very loop-de-loopy even for internal implementation stuff.

std::formatter<bool, CharT> formatter;
    // ok cool so this knows how to format bools?
auto format_ctx = FormatCtxT(out, std::make_format_args<FormatCtxT>(arg));
    // we're passing in `arg` here, so now it's formatted?
formatter.format(arg, format_ctx);
    // no, ok, pass in `arg` AGAIN, third time's the charm!
52–55 ↗(On Diff #354742)

This was a vestige of the time when the C++20 concepts weren't in trunk yet, right?
I believe every instance of this pattern should be replaced with a one-liner:

static_assert(std::semiregular<std::formatter<bool, CharT>>);
test(...

In fact, consider moving the assertion up into test, where you already have a variable of the correct type, so you could just say

static_assert(std::semiregular<decltype(formatter)>);

and wouldn't have to worry about typos potentially introduced by faulty cut-and-paste.

libcxx/test/std/utilities/format/format.formatter/format.formatter.spec/formatter.c_string.pass.cpp
47–55 ↗(On Diff #354742)

pointer is rightly named but unnecessary; you could just requires is_pointer_v<CharPtr> (or in fact do nothing at all, because this function doesn't need to be constrained for any technical reason). object is misnamed, since it is significantly different from std::is_object_v (but also, it's unused). Write instead:

template<class CStringT>
void test() {
    using CharT = std::remove_cv_t<std::remove_pointer_t<CStringT>>;
        // alternatively, = std::iter_value_t<CStringT>;
    static_assert(std::semiregular<std::formatter<CStringT, CharT>>);
    test<CStringT>(STR(" azAZ09,./<>?"), STR("}"), CSTR(" azAZ09,./<>?"));

    std::basic_string<CharT> s(CSTR("abc\0abc"), 7);
    test<CStringT>(STR("abc"), STR("}"), s.c_str());
}

I think what I've got for the embedded-NUL test ought to work. I have not tried it.

80–88 ↗(On Diff #354742)

I strongly recommend flattening these:

int main(int, char**) {
  test<char*>();
  test<const char*>();
  test<wchar_t*>();
  test<const wchar_t*>();
}

Even if you don't flatten them, at least use different names for test (the actual test) and test (the thing that calls test twice). I don't know our convention off the top of my head, but I know libcxx/test/ has plenty of examples of this to set naming precedent.

libcxx/test/std/utilities/format/format.formatter/format.formatter.spec/formatter.const_char_array.pass.cpp
55–60 ↗(On Diff #354742)

I think what you want here is

std::formatter<ArrayT, CharT> formatter;
  [...]
CharT array[N];
std::copy(text, text + N, array);
ArrayT& arg = array;
auto format_ctx = FormatCtxT(std::back_inserter(result), std::make_format_args<FormatCtxT>(arg));
formatter.format(arg, ctx);

Note consistent use of the names FormatCtxT, arg, etc. among all the test cases.
I admit I don't understand why you're doing the weird thing with the class-type NTTP Tester here; couldn't you just hard-code N in the test case? We don't need to "deduce the size of the input" when there is only one input. And then you can get rid of a lot of "magic" — no more CTAD/CNTTPs, no more __builtin_memcpy, etc.

libcxx/test/std/utilities/format/format.functions/tests.inc
28–33

IIUC, these tests are testing the wording of libc++-specific what-strings that are not mandated by the Standard, is that right? If so, they belong in libcxx/test/libcxx/.
Or perhaps it would be better to leave the tests here, but put every instance of assert(e.what() == what) under a libc++-specific #ifdef. That way, on non-libc++ libraries, we'd test that some exception was correctly thrown; we just wouldn't test its what-string.

libcxx/test/std/utilities/format/format.functions/vformat_to.pass.cpp
37–42

Please don't provide ==s for library types, not even in .cpp files. In this case your initial size check can be omitted anyway, because std::equal can do it: just change e.g. line 68 to

assert(std::equal(out.begin(), out.end(), expected.begin(), expected.end()));
Mordante marked 7 inline comments as done.Jul 6 2021, 11:09 AM

Thanks for all the review comments!

libcxx/test/std/utilities/format/format.formatter/format.formatter.spec/formatter.bool.pass.cpp
29–43 ↗(On Diff #349177)

I agree this example is quite verbose, but in the format function it isn't that ugly. There the parsing and formatting for an argument looks like

formatter<decltype(__arg), _CharT> __formatter;
__parse_ctx.advance_to(__formatter.parse(__parse_ctx));
__ctx.advance_to(__formatter.format(__arg, __ctx));
52–55 ↗(On Diff #354742)

This was a vestige of the time when the C++20 concepts weren't in trunk yet, right?

Correct!

I believe every instance of this pattern should be replaced with a one-liner:

static_assert(std::semiregular<std::formatter<bool, CharT>>);
test(...

Good point, this indeed seems to be the case.

In fact, consider moving the assertion up into test, where you already have a variable of the correct type, so you could just say

static_assert(std::semiregular<decltype(formatter)>);

and wouldn't have to worry about typos potentially introduced by faulty cut-and-paste.

This seems to be the only one using a hard-coded type instead of template arguments. But avoiding possible copy-paste issues always is a good thing(tm). Especially now the four static_asserts can be replaced by just one.

libcxx/test/std/utilities/format/format.formatter/format.formatter.spec/formatter.c_string.pass.cpp
47–55 ↗(On Diff #354742)

pointer is rightly named but unnecessary; you could just requires is_pointer_v<CharPtr> (or in fact do nothing at all, because this function doesn't need to be constrained for any technical reason). object is misnamed, since it is significantly different from std::is_object_v

This point is no longer needed, since I flattened the tests as you suggested in another comment.

(but also, it's unused). Write instead:

template<class CStringT>
void test() {
    using CharT = std::remove_cv_t<std::remove_pointer_t<CStringT>>;
        // alternatively, = std::iter_value_t<CStringT>;
    static_assert(std::semiregular<std::formatter<CStringT, CharT>>);

This one is move to the real tester using the decltype as you suggested.

test<CStringT>(STR(" azAZ09,./<>?"), STR("}"), CSTR(" azAZ09,./<>?"));

std::basic_string<CharT> s(CSTR("abc\0abc"), 7);
test<CStringT>(STR("abc"), STR("}"), s.c_str());

}

I think what I've got for the embedded-NUL test ought to work. I have not tried it.

That's actually a nice looking method, thanks for the suggestion!
I tested it and it passes the tests locally.

libcxx/test/std/utilities/format/format.formatter/format.formatter.spec/formatter.const_char_array.pass.cpp
55–60 ↗(On Diff #354742)

The formatter type is template<size_t N> struct formatter<const charT[N], charT>; The size of N isn't fixed in this test and future tests in the series add more sizes. Since this works I prefer to keep it like this.

I can have a look at the consistency, but the tests already have some minor differences between them.

libcxx/test/std/utilities/format/format.functions/tests.inc
28–33

I guarded them with #ifdef _LIBCPP_VERSION. I prefer to keep them since they've been a great aid during debugging and verification.

libcxx/test/std/utilities/format/format.functions/vformat_to.pass.cpp
37–42

I'm not objecting against this change, but what's the reason you done like operator== for library types?

Mordante updated this revision to Diff 356791.Jul 6 2021, 11:39 AM
Mordante marked 6 inline comments as done.

Addresses review comments.

Quuxplusone added inline comments.Jul 6 2021, 3:14 PM
libcxx/test/std/utilities/format/format.functions/vformat_to.pass.cpp
37–42

Combination of two premises:
(1) Users shouldn't add operator== for types they don't own, and users don't own STL types. (If someone filed a bug report and attached this code, saying "libc++ misbehaves if I add my own operator== between vector and string!"— well, we'd just close it as NOTABUG, right?)
(2) The libc++ test suite should contain only such code as our users might legitimately write. Otherwise what would we really be testing here?— we'd be testing that libc++ works if the user provides an operator== between vector and string— but literally no real user ever does that, nor are they allowed to do it, so testing that configuration isn't meaningful. Or at least the test isn't as meaningful as it could be.

I suspect the controversial premise is more likely (2) than (1), but anyway I tried to explain both; and I think you'd agree that if you accept both (1) and (2), then it follows that the libc++ test suite shouldn't add extra operator==s for STL types.

Mordante marked an inline comment as done.Jul 7 2021, 11:39 AM
Mordante added inline comments.
libcxx/test/std/utilities/format/format.functions/vformat_to.pass.cpp
37–42

Thanks for the information. I fully agree with (2). However I wasn't aware that (1) is a rule. I've seen operator overloading for standard library types quite often, for example operator<< for the stream objects.

Even if don't fully agree with (1) I still feel your solution of using a standard algorithm is better then writing the operator overload.

Mordante updated this revision to Diff 357274.Jul 8 2021, 10:46 AM
Mordante marked an inline comment as done.

Rebase.
Use new monostate header.
Use LIBCPP_ASSERT.

vitaut accepted this revision.Jul 10 2021, 9:28 AM

Looks great, just a few nits inline. Accepting the diff mostly as a reminder for myself that I've already looked at it.

libcxx/include/__format/format_string.h
100–101

This could be a static assert.

libcxx/include/__format/formatter.h
37

nit: "conform" -> "conform to"

libcxx/include/format
603

nit: I'd call it __handle_replacement_field for consistency with the standard naming.

682

I would call it something like "unmatched '}'" rather than "an invalid escape sequence" because we don't know whether the intent was to have an escape sequence.

vitaut added inline comments.Jul 10 2021, 9:28 AM
libcxx/include/format
701

I don't think you need to pass __args.__size() here because it is for compile-time checks only.

815

same here

vitaut added inline comments.Jul 10 2021, 9:38 AM
libcxx/test/std/utilities/format/format.formatter/format.formatter.spec/formatter.bool.pass.cpp
38 ↗(On Diff #357274)

I'd add a check that the return value points to the end of the format string here and elsewhere.

51–52 ↗(On Diff #357274)

I'd test that empty string is also parsed correctly which is an important corner case.

libcxx/test/std/utilities/format/format.formatter/format.formatter.spec/formatter.floating_point.pass.cpp
58 ↗(On Diff #357274)

Why do you have "}" in other tests and "{}" here?

Mordante marked 7 inline comments as done.Jul 11 2021, 6:14 AM

Thanks a lot for your review!

libcxx/include/__format/format_string.h
100–101

Good catch!

libcxx/include/format
682

I like your reasoning, I've changed it.

701

But doesn't that mean I need to add it again when I start working on P2216?

libcxx/test/std/utilities/format/format.formatter/format.formatter.spec/formatter.bool.pass.cpp
38 ↗(On Diff #357274)

AFAIK this doesn't need to point to the end of the format string. Based on earlier comments and wording of http://eel.is/c++draft/format#tab:formatter

Parses format-spec ([format.string]) for type T in the range [pc.begin(), pc.end()) until the first unmatched character.
Throws format_­error unless the whole range is parsed or the unmatched character is }.
[Note 1: This allows formatters to emit meaningful error messages.
— end note]
Stores the parsed format specifiers in *this and returns an iterator past the end of the parsed range.

I made changes to this function. It now either returns an iterator to fmt.end() or an iterator at the closing } of the replacement-field.

51–52 ↗(On Diff #357274)

In other patches, for example` D103433. I added an extra layer to test both "}" and "" from one tester. I applied the same method to the tests in this directory. I also changed the names of the test functions to not use the overload resolution to pick the proper function. When we're happy with that approach I'll use these improvements for later patches in the series.

libcxx/test/std/utilities/format/format.formatter/format.formatter.spec/formatter.floating_point.pass.cpp
58 ↗(On Diff #357274)

This should also be "}", this was an older approach. The floats haven't had as much love as the other implemented types yet. (Mainly since for floats I still need to get std::to_chars working properly.)

Mordante updated this revision to Diff 357796.Jul 11 2021, 6:25 AM
Mordante marked 5 inline comments as done.

Addresses the review comments.

Mordante updated this revision to Diff 357801.Jul 11 2021, 7:51 AM

Rebase, hopefully fixes Arm builds.
Fixes UB in tests detected by the debug and UBSAN builds.

vitaut added inline comments.Jul 11 2021, 10:41 AM
libcxx/test/std/utilities/format/format.formatter/format.formatter.spec/formatter.bool.pass.cpp
38 ↗(On Diff #357274)

Good point.

29–43 ↗(On Diff #349177)

is this really the most ergonomic way to fit all these puzzle pieces together?

Looks OK to me since it's only a test . These are rarely used in user code, mostly when reusing formatters which is much easier.

Mordante updated this revision to Diff 358844.Jul 14 2021, 11:16 PM

rebased
updated to changes in the std::basic_format_context
Mark some test libc++ only
Use private module headers

Mordante updated this revision to Diff 359639.Jul 18 2021, 11:13 AM

Guard unit tests for implementation details

Mordante added inline comments.Jul 20 2021, 10:13 AM
libcxx/include/__format/format_string.h
47

s/_LIBCPP_INLINE_VISIBILITY/_LIBCPP_HIDE_FROM_ABI/ here and all other occurrences in this patch.

Mordante updated this revision to Diff 361450.Jul 24 2021, 5:41 AM

s/_LIBCPP_INLINE_VISIBILITY/_LIBCPP_HIDE_FROM_ABI/
Guard better against ADL
Move internal unit tests
Generate private module tests
Disable the floating-point stub, since the series has no replacement

libcxx/include/__format/format_string.h
131

s/values/value/
and please don't line-break

libcxx/include/format
447

s/128 bit/128-bit/g

625–626

Style tip: When you break the line like this, it makes it harder for a maintainer to git grep __throw_format_error.*terminate when they're trying to track down where a given error was thrown. (Although line-breaking in the middle of a string is of course infinitely worse!)
In this case there's no reason to line-break at all; just join lines 630 and 631. Ideally, every single time you throw a format_error, it'll be on a line of the form

[ ]*__throw_format_error(".*");

(Tangent: I would not object if you made it _VSTD::__throw_format_error throughout. You're right that the string literal won't trigger ADL, so ADL-proofing isn't necessary; but it might help visually distinguish this free function from a member function.)

643

/misses the/is missing a/

653–654
  • Don't break in the middle of this error message.
  • This function is called in only two places; I recommend inlining it in those places. Then you can improve the message by customizing it to the specific situation being parsed.
  • If you do keep it as a free function (which I don't recommend), please _VSTD:: its calls, and consider giving it a name that (1) doesn't look so much like the uglified version of std::next, (2) involves the word format.
682

s/charater/character/

687–688

Lines 692, 677, 657: I question the usefulness of these [[unlikely]] attributes. Compilers are generally very good at determining that the throwing path is the unlikely path, already; and this seems like cluttering up our code for no practical purpose.
(Ditto [[nodiscard]] on non-user-visible functions, such as on lines 45, 63, 70, 80; ditto noexcept on non-user-visible functions that are never evaluated for noexceptness; ditto constexpr on non-user-visible-functions that are never called in a constexpr context.)

Mordante marked 7 inline comments as done.Jul 25 2021, 6:52 AM

Thanks for the review comments!

libcxx/include/format
625–626

This change is automatically done by clang-format since I've still limited it to 80 characters instead of 120 as our official standard is. This since parts of this series were written before the limit was adjusted. After all outstanding patches are merged I want to apply our real coding style with clang-format, this will fix it. I don't want to do that now, since it will result in merge conflicts.

I don't mind using _VSTD::__throw_format_error but currently all calls to __throw_xxx functions use this style. So when I change it here I want to create a separate patch to do the rest of our code base. @ldionne what's your preference?

643

I changed it to 'misses a', this is more in line with the other messages in this file.

653–654

Again this is what clang-format does. When we don't like it we should adjust its settings to the format we want.
Originally this function was used in more places, but addressing other review comments reduced the number of use cases.
With only two users it indeed seems better to inline them. This allows for a small code improvement at the second call site.

687–688

I wasn't aware compilers already optimize the throwing path as unlikely. I tested it on compiler explorer and it indeed seems to be the case.
I like [[nodiscard]] for two reasons:

  • It avoids accidental bugs by not evaluating.
  • Documents the intended usage.

Even if a function isn't used in constexpr context it can be useful. I've used it during testing and suddenly UB became a compilation error.

So I don't consider [[nodiscard]], noexcept and constexpr as clutter.

Mordante updated this revision to Diff 361503.Jul 25 2021, 7:33 AM
Mordante marked 4 inline comments as done.

Addresses review comments.

Mordante added inline comments.Jul 29 2021, 9:36 AM
libcxx/include/format
687–688

I had a talk with @ldionne and he also prefers to remove the extra [[nodiscard]]s. He mention they've also been removed in the ranges code, which previously used the same style as I did here. So we agreed to remove them.

Mordante updated this revision to Diff 362813.Jul 29 2021, 9:56 AM

Rebased on main and updated to work against main.
Remove [[nodiscard]]s

ldionne requested changes to this revision.Aug 31 2021, 11:58 AM

Got a few comments on the tests, but apart from that this mostly LGTM. I'm assuming that the several TODOs are handled in subsequent patches.

libcxx/include/__format/formatter.h
52

Why is this _LIBCPP_HIDDEN and not _LIBCPP_HIDE_FROM_ABI?

libcxx/test/std/utilities/format/format.functions/format_to_n.pass.cpp
12

You can remove mentions of gcc-10 everywhere now.

Also, it would be great to have a comment explaining why this is unsupported on gcc-11 (in all places where you do that).

libcxx/test/std/utilities/format/format.functions/tests.inc
19

Instead of using this inclusion technique, I would rather see this done this way:

// TestFunction must be callable as check(expected-result, string-to-format, args-to-format...)
// ExceptionTest must be callable as check_exception(expected-exception, string-to-format, args-to-format...)
template <class CharT, class TestFunction, class ExceptionTest>
void format_tests(TestFunction check, ExceptionTest check_exception) {
  check(STR("hello 01"), STR("hello {0:}{1:}"), false, true);
  // ...
}

Then, from the various tests, you can do:

#include "format_tests.h"

auto test = []<class CharT, class... Args>(std::basic_string<CharT> expected, std::basic_string<CharT> fmt, const Args&... args) {
  // ...
};

auto test_exception = []<class CharT, class... Args>(std::string_view what, std::basic_string<CharT> fmt, const Args&... args) {
  // ...
};

int main(int, char**) {
  format_tests<char>(test, test_exception);
  format_tests<wchar_t>(test, test_exception);

  return 0;
}

IMO this is a less surprising way to reuse code and that makes it easier to understand what's happening when you look at the test in isolation. WDYT?

libcxx/test/std/utilities/format/format.functions/vformat.locale.pass.cpp
43

This should be TEST_HAS_NO_EXCEPTIONS (elsewhere too).

libcxx/test/std/utilities/format/format.functions/vformat.pass.cpp
25

Those comments are not especially useful IMO, we could remove them altogether.

This revision now requires changes to proceed.Aug 31 2021, 11:58 AM
Mordante marked 5 inline comments as done.Sep 1 2021, 8:48 AM

Got a few comments on the tests, but apart from that this mostly LGTM. I'm assuming that the several TODOs are handled in subsequent patches.

Part of them will be done in the patches in this series. Others will be done in not yet finished patches. But the intention is to solve them all before marking the <format> header as complete.

libcxx/include/__format/formatter.h
52

Not sure, but fixed it.

libcxx/test/std/utilities/format/format.functions/format_to_n.pass.cpp
12

I need to investigate why gcc-11 doesn't work. I've added a TODO to look at it later. I'll need to update one of my systems so I have gcc-11 locally which makes testing easier.

libcxx/test/std/utilities/format/format.functions/tests.inc
2

This file is obsolete and should be removed. I prefer to keep this file a little bit longer. This will make it a lot easier to migrate the other patches in the series to the new format_tests.h. Once that migration is done this file can be removed.

19

I like this solution a lot! Thanks for the suggestion.

libcxx/test/std/utilities/format/format.functions/vformat.pass.cpp
25

I feel they are useful due to the usage of the tests.inc. But since that file will be replaced with your suggested format_tests.h I agree that they are not needed.

Mordante updated this revision to Diff 369976.Sep 1 2021, 10:23 AM
Mordante marked 4 inline comments as done.

Rebased.
Addresses review comments.

Mordante updated this revision to Diff 369982.Sep 1 2021, 10:39 AM

Guard against min/max macros.

ldionne accepted this revision.Sep 3 2021, 12:04 PM
ldionne added inline comments.
libcxx/test/std/utilities/format/format.functions/format_to_n.pass.cpp
12

FWIW, you can use our Docker image to match the CI environment if you'd like:

./libcxx/utils/ci/run-buildbot-container # this will run libc++'s Docker image that we use in CI, which contains GCC 11 amongst other things
$ ./libcxx/utils/ci/run-buildbot generic-gcc # run this from the Docker image
libcxx/test/std/utilities/format/format.functions/tests.inc
2

It's very unusual to approve patches with these sorts of artifacts, let's make this an exception.

This revision is now accepted and ready to land.Sep 3 2021, 12:04 PM
Mordante added inline comments.Sep 4 2021, 2:40 AM
libcxx/test/std/utilities/format/format.functions/tests.inc
2

Thanks. That really helped to migrate the other patches to the new format_tests.h without introducing merge conflicts in the process.

This revision was automatically updated to reflect the committed changes.
vitaut added inline comments.Sep 5 2021, 7:19 AM
libcxx/include/format
734

I know this diff has already landed but you definitely don't want to inline the top-level "v" functions such as vformat to reduce per-call code bloat (see e.g. https://godbolt.org/z/f8c1P5bP5).

Mordante added inline comments.Sep 5 2021, 8:31 AM
libcxx/include/format
734

Correct. I haven't spend much time reducing the the code bloat, but it's on my TODO list. At the moment I'm focusing on implementing the papers and LWG-issues. (Actually I'm working on reducing the code bloat required by P2216 ;-))

I added this remark to my TODO list.