Page MenuHomePhabricator

[libc++][format] Add basic_format_parse_context.
ClosedPublic

Authored by Mordante on Dec 12 2020, 11:04 AM.

Details

Summary

Implements parts of:

  • P0645 Text Formatting

Depends on D92214

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Herald added a project: Restricted Project. · View Herald TranscriptDec 12 2020, 11:04 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Mordante updated this revision to Diff 311404.Dec 12 2020, 12:06 PM

Fixes a GCC shadow warning.

curdeius requested changes to this revision.Dec 13 2020, 5:22 AM

You should mark tests that use format_error with // XFAIL: with_system_cxx_lib=macosx10.9|...|15.

libcxx/include/format
134

Couldn't we add at least a debug check to verify "Preconditions: end() is reachable from it."?
http://eel.is/c++draft/format#parse.ctx-5

154

Why is_constant_evaluated? Does it correspond to the phrase "Remarks: Call expressions where id >= num_args_ are not core constant expressions ([expr.const])."?
http://eel.is/c++draft/format#parse.ctx-11
Could you explain how it should be understood?

161

Why not enum class?

libcxx/test/std/utilities/format/format.formatter/format.parse.ctx/ctor.pass.cpp
34

Please add a test for the ctor being explicit (using test_convertible for instance).

libcxx/test/std/utilities/format/format.formatter/format.parse.ctx/types.compile.pass.cpp
48

I think you should guard it on __cpp_char8_t.

This revision now requires changes to proceed.Dec 13 2020, 5:22 AM
Mordante marked 5 inline comments as done.Dec 14 2020, 7:09 AM

Thanks for the review!

I'll add the XFAILs to the next version of the patch.

libcxx/include/format
134

I had a look at the debug iterators and adding this test seems not trivial. So I think it's not worth the effort to add it here.

154

Since http://eel.is/c++draft/format#parse.ctx-10 doesn't specify this exception to be thrown I read this that the test only should be done when std::is_constant_evaluated is true. In that case the function should no longer be a constexpr function. So I use the throw to achieve this. I'm not sure why the exception just shouldn't be thrown unconditionally, I hope to figure that out when I'm a bit further with the implementation.

I'll add some comment in the next revision of the patch.

161

I didn't add it since the enum is only used internally in the class so I feel the class doesn't add to much benefits and http://eel.is/c++draft/format.parse.ctx doesn't require it.

If you feel strongly about it I don't mind changing it.

libcxx/test/std/utilities/format/format.formatter/format.parse.ctx/types.compile.pass.cpp
48

I thought it wouldn't be required, but I'll add _LIBCPP_NO_HAS_CHAR8_T and _LIBCPP_HAS_NO_UNICODE_CHARS guards.

Mordante updated this revision to Diff 311591.Dec 14 2020, 7:32 AM
Mordante marked 4 inline comments as done.

Addresses review comments.

I haven't finished reviewing. Will do soon.

libcxx/include/format
161

Don't bother. No strong feelings about this :).

curdeius added inline comments.Dec 15 2020, 12:35 AM
libcxx/include/format
144

I see that it isn't checked that __next_arg_id_ < __num_args.
fmtlib has the following comment (https://github.com/fmtlib/fmt/blob/5a493560f59369e9fa664e8945b8e8a8ec4391b2/include/fmt/core.h#L599):

// Don't check if the argument id is valid to avoid overhead and because it
// will be checked during formatting anyway.

I don't know what's the libc++'s policy to use assertions, but for me it's a good place to put a _LIBCPP_ASSERT.

162

Same as above, I'd expect at least an assertion in debug mode (in non-constexpr context).

libcxx/test/std/utilities/format/format.formatter/format.parse.ctx/check_arg_id.pass.cpp
65

I don't see why you can't test the first part of test_exception in constexpr context. Changing return type to bool and returning false/true where appropriate should be enough to make it work.
Obviously, the part with test_arg can't be tested there.

libcxx/test/std/utilities/format/format.formatter/format.parse.ctx/next_arg_id.pass.cpp
55

As above, but here you can test everything in constexpr context.

curdeius added inline comments.Dec 15 2020, 12:48 AM
libcxx/include/format
161

FYI, your implementation seems to be exactly what the author meant (std branch is missing in the main fmt repo):
https://github.com/agmt/fmt/blob/std/include/format#L181-L199

Mordante marked 2 inline comments as done.Dec 15 2020, 9:22 AM
Mordante added inline comments.
libcxx/include/format
144

I didn't add a _LIBCPP_ASSERT since it indeed can be tested during formatting. If the index is out of bounds basic_format_args::get will return a default created basic_format_arg (http://eel.is/c++draft/format#args-4) This means the object has a std::monostate as value. In my WIP code I have a formatter for a std::monostate which will throw an exception.

I think it's better to throw an exception to inform the user about errors and I think it's not good to change that behaviour with debug macros. Note for other parts in my WIP code I use _LIBCPP_ASSERT to validate the expected state, but if they trigger it means the library code isn't robust enough.

@ldionne What do you think about the usage of _LIBCPP_ASSERT ?

libcxx/test/std/utilities/format/format.formatter/format.parse.ctx/check_arg_id.pass.cpp
65

I'm not entirely sure what you mean. The line context.next_arg_id(); will call __format::__throw_error which is not a constexpr function, turning it into one will fail with the following diagnostic error: constexpr function never produces a constant expression [-Winvalid-constexpr]. Can you explain what change you think should work.

I made the separate function test_exception for the parts which can't be tested as constexpr.

curdeius accepted this revision.Dec 15 2020, 1:03 PM

Apart from the question whether adding assertion fits here, this LGTM.

libcxx/test/std/utilities/format/format.formatter/format.parse.ctx/check_arg_id.pass.cpp
65

You're completely right. I must have been confused when writing this:).

libcxx/test/std/utilities/format/format.formatter/format.parse.ctx/next_arg_id.pass.cpp
55

Please ignore the above too.

Mordante updated this revision to Diff 312968.Dec 20 2020, 4:17 AM
Mordante marked an inline comment as done.

Replace a std:: with _VSTD::

Mordante planned changes to this revision.Jan 22 2021, 8:35 AM
Mordante updated this revision to Diff 319609.Jan 27 2021, 9:58 AM

Update the synopsis after the changes in D92214.
Ran clang-format.
Rebased.

Mordante updated this revision to Diff 319617.Jan 27 2021, 10:33 AM

Fix build breakage due to clang-format. In C++03 mode it breaks string prefixes like u8"foo".

ldionne accepted this revision.Jan 28 2021, 8:24 AM

This LGTM, but I would prefer if we stuck to the current convention and used std::__throw_format_error instead. Not a blocking comment.

This revision is now accepted and ready to land.Jan 28 2021, 8:24 AM

This LGTM, but I would prefer if we stuck to the current convention and used std::__throw_format_error instead. Not a blocking comment.

Thanks for the review! My diversion of the convention was up for discussion ;-) So I'll switch back to the convention and use std::__throw_format_error. I'll commit after it passes CI.

Mordante updated this revision to Diff 319893.Jan 28 2021, 9:17 AM
Mordante edited the summary of this revision. (Show Details)

Use __throw_format_error instead of __format::__throw__error.

Mordante updated this revision to Diff 320488.Feb 1 2021, 8:46 AM

Rebase to trigger CI.

This revision was automatically updated to reflect the committed changes.

Reverted in 68f66f37d7d7 because of the build break mentioned inline, let me know if you need help to reproduce!

libcxx/include/format
159

We have a build breakage on bootstrapping clang here:

In file included from /var/lib/buildkite-agent/builds/buildkite-69fdf6c495-wt2bd-1/mlir/mlir-core/libcxx/src/format.cpp:9:
/tmp/ci-nGNyLRM9V3/include/c++/v1/format:153:16: error: no member named 'is_constant_evaluated' in namespace 'std::__1'
    if (_VSTD::is_constant_evaluated() && __id >= __num_args_)
        ~~~~~~~^
1 error generated.

See https://buildkite.com/mlir/mlir-core/builds/11261#3cc923a2-b03c-4f8b-a0f1-83d638fff493

Some additional information that might help: looks like std:: is_constant_evaluated is supported since Clang 9: https://clang.llvm.org/cxx_status.html

Some additional information that might help: looks like std:: is_constant_evaluated is supported since Clang 9: https://clang.llvm.org/cxx_status.html

Thanks there's already a library macro to test whether it's available _LIBCPP_HAS_NO_BUILTIN_IS_CONSTANT_EVALUATED. But the we don't test with old compilers in C++20 mode.

Mordante reopened this revision.Feb 2 2021, 9:24 AM
This revision is now accepted and ready to land.Feb 2 2021, 9:24 AM
Mordante updated this revision to Diff 320816.Feb 2 2021, 9:24 AM

This should fix building with clang-8 which doesn't support
std::is_constant_evaluated().

@mehdi_amini Can you test whether this fixes the MLIR build?

dmajor added a subscriber: dmajor.Feb 2 2021, 10:25 AM
Mordante updated this revision to Diff 321926.Feb 6 2021, 4:19 AM

Rebase to trigger CI.

Mordante updated this revision to Diff 322704.Feb 10 2021, 8:56 AM

Use concept support to disable <format> on older compilers. Note this is intended to be a temporary solution until libc++ requires a modern compiler.
Rebased.

This revision was automatically updated to reflect the committed changes.
mehdi_amini added a comment.EditedFeb 11 2021, 9:55 AM

I still see an error unfortunately:

In file included from /var/lib/buildkite-agent/builds/buildkite-69fdf6c495-8wxf7-1/mlir/mlir-core/libcxx/src/format.cpp:9:
/tmp/ci-6kGTAaqbDL/include/c++/v1/format:159:16: error: no member named 'is_constant_evaluated' in namespace 'std::__1'
    if (_VSTD::is_constant_evaluated() && __id >= __num_args_)
        ~~~~~~~^

Actually I see that you pushed a follow up fix? Kicked another build here: https://buildkite.com/mlir/mlir-core/builds/11503

Fails differently with the forward fix:

libcxx/src/format.cpp:15:1: error: use of undeclared identifier 'format_error'; did you mean 'domain_error'?
format_error::~format_error() noexcept = default;
^~~~~~~~~~~~
domain_error
/tmp/ci-yIcukneKik/include/c++/v1/stdexcept:122:29: note: 'domain_error' declared here
class _LIBCPP_EXCEPTION_ABI domain_error
                            ^
/var/lib/buildkite-agent/builds/buildkite-69fdf6c495-8wxf7-1/mlir/mlir-core/libcxx/src/format.cpp:15:16: error: expected the class name after '~' to name a destructor
format_error::~format_error() noexcept = default;
               ^~~~~~~~~~~~
               domain_error

It seems modifying the header and building libc++ doesn't rebuild the library. So it seemed to work for me. After I touched format.cpp I could reproduce the issue. Pushed another fix that works for me locally, but I'll keep an eye on the MLIR build server.

It passed the host phase :)

The MLIR build is now green :-)

Thanks for checking on it! :)