This is an archive of the discontinued LLVM Phabricator instance.

[libc++][format] Improves diagnostics.
ClosedPublic

Authored by Mordante on Jun 10 2023, 9:47 AM.

Details

Reviewers
ldionne
Group Reviewers
Restricted Project
Commits
rG402eb2ef099f: [libc++][format] Improves diagnostics.
Summary

Improves both the compile-time and run-time errors.
At compile-time it does a bit more work to get more specific errors.
This could be done at run-time too, but that has a performance penalty.
Since it's expected most use-cases use format* instead of vformat* the
compile-time errors are more common.

For example when using

std::format_to("{:-c}", 42);

Before compile output would contain

std::__throw_format_error("The format-spec should consume the input or end with a '}'");

Now it contains

std::__throw_format_error("The format specifier does not allow the sign option");

Given a better indication the sign option is not allowed. Note the
output is still not user-friendly; C++ doesn't have good facilities to
generate nice messages from the library.

In general all messages have been reviewed and improved, using a more
consistent style and using less terms used in the standard. For example

format-spec -> format specifier
arg-id -> argument index

Diff Detail

Event Timeline

Mordante created this revision.Jun 10 2023, 9:47 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 10 2023, 9:47 AM
Herald added a subscriber: arphaman. · View Herald Transcript
Mordante updated this revision to Diff 530293.Jun 11 2023, 4:45 AM

CI fixes.

Mordante updated this revision to Diff 530294.Jun 11 2023, 5:41 AM

CI fixes.

Mordante updated this revision to Diff 530297.Jun 11 2023, 7:09 AM

CI fixes.

Mordante updated this revision to Diff 530299.Jun 11 2023, 7:43 AM

CI fixes.

Mordante updated this revision to Diff 532887.Jun 20 2023, 6:19 AM

Rebased and minor polishing.

Mordante published this revision for review.Jun 20 2023, 10:49 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 20 2023, 10:49 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
ldionne added inline comments.
libcxx/include/__format/format_functions.h
131

Elsewhere too.

Could you split this patch into a patch that changes the error messages (e.g. reformulation, capitalization, etc), another one that adds additional parsing logic to improve the error messages and one that changes the bracket handling? I feel like there's more than one orthogonal things in this patch, especially since part of it requires an ABI change that goes mostly unnoticed otherwise.

libcxx/include/__format/parser_std_format_spec.h
384–385

Can you use braces consistently after the else? Otherwise it becomes really hard to distinguish what is in the else given the nesting.

444–445
Mordante marked 2 inline comments as done.Jul 15 2023, 11:42 AM
Mordante updated this revision to Diff 540721.Jul 15 2023, 12:00 PM

Addresses review comment and rebased on the patches splitted from this patch.

ldionne accepted this revision.Jul 18 2023, 10:20 AM
This revision is now accepted and ready to land.Jul 18 2023, 10:20 AM
This revision was automatically updated to reflect the committed changes.