This is an archive of the discontinued LLVM Phabricator instance.

[libc++][format] Adds char formatter.
ClosedPublic

Authored by Mordante on Jun 1 2021, 9:20 AM.

Details

Reviewers
curdeius
ldionne
miscco
vitaut
Group Reviewers
Restricted Project
Commits
rG49e736d845d8: [libc++][format] Adds char formatter.
Summary

Implements the formatter for all fundamental integer types.
[format.formatter.spec]/2.1
The specializations

template<> struct formatter<char, char>;
template<> struct formatter<char, wchar_t>;
template<> struct formatter<wchar_t, wchar_t>;

This removes the stub implemented in D96664.

Implements parts of:

  • P0645 Text Formatting

Diff Detail

Event Timeline

Mordante created this revision.Jun 1 2021, 9:20 AM
Mordante requested review of this revision.Jun 1 2021, 9:20 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 1 2021, 9:20 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Mordante updated this revision to Diff 349770.Jun 3 2021, 10:50 PM

Rebase.
Disable a failing test on Windows.

Mordante edited the summary of this revision. (Show Details)Jun 5 2021, 4:17 AM
Mordante added reviewers: curdeius, ldionne, miscco, vitaut.
Mordante planned changes to this revision.Jun 27 2021, 5:38 AM

This patch will be updated due to changes in other patches in the series.

Mordante updated this revision to Diff 355558.Jun 30 2021, 8:15 AM
Mordante edited the summary of this revision. (Show Details)

Rebased and updated:

  • Address some changes from previous patches
  • Minor cleanups and comment improvements
  • Added a module map
vitaut added inline comments.Jul 17 2021, 8:59 AM
libcxx/test/std/utilities/format/format.functions/tests.inc
113

This should probably be fixed in a different diff but . alone is not a prevision field at all: http://eel.is/c++draft/format.string.std.

vitaut added inline comments.Jul 17 2021, 9:09 AM
libcxx/test/std/utilities/format/format.functions/tests.inc
113

AFAICS this should give "The format-spec precision field doesn't contain a value or arg-id" according to the parsing logic in https://reviews.llvm.org/D103368.

Mordante added inline comments.Jul 18 2021, 10:30 AM
libcxx/test/std/utilities/format/format.functions/tests.inc
113

Due to https://reviews.llvm.org/D103368#inline-990725 this will no longer mention precision in the exception message. I was still addressing other review comments so haven't gotten around to update this patch.

Mordante updated this revision to Diff 359636.Jul 18 2021, 10:32 AM

Rebase
Use private module headers
Guard unit tests for implementation details

Mordante added inline comments.Jul 20 2021, 10:14 AM
libcxx/include/__format/formatter_char.h
46

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

Mordante planned changes to this revision.Jul 30 2021, 4:22 AM

Needs updates due to changes in D103368.

Mordante updated this revision to Diff 364492.Aug 5 2021, 8:20 AM

Rebased and adjusted for chandges in main
s/_LIBCPP_INLINE_VISIBILITY/_LIBCPP_HIDE_FROM_ABI/
Generate private module tests

vitaut added a comment.Aug 8 2021, 8:53 AM

LGTM

libcxx/test/libcxx/utilities/format/format.string/format.string.std/std_format_spec_char.pass.cpp
174–175 ↗(On Diff #364492)

You already have this assert above.

Mordante marked 3 inline comments as done.Aug 10 2021, 11:23 AM
Mordante added inline comments.
libcxx/test/libcxx/utilities/format/format.string/format.string.std/std_format_spec_char.pass.cpp
174–175 ↗(On Diff #364492)

True, but I like these kind of asserts closely "attached" to where they're needed.

Mordante updated this revision to Diff 370737.Sep 4 2021, 6:41 AM
Mordante marked an inline comment as done.

Rebased and adjusted for changes in main
s/_LIBCPP_INLINE_VISIBILITY/_LIBCPP_HIDE_FROM_ABI/

Mordante updated this revision to Diff 372965.Sep 16 2021, 8:48 AM

Remove the unneeded _LIBCPP_PUSH_MACROS.
Updated code to changes earlier in the patch series.

ldionne accepted this revision.Oct 6 2021, 12:28 PM

For lurkers -- I'm going through these patches and looking out for libc++ specific things. We agreed with @Mordante to rely on @vitaut 's review and the test coverage here for the correctness of the actual code, which is why I have so few comments.

libcxx/test/libcxx/utilities/format/format.string/format.string.std/std_format_spec_char.pass.cpp
437 ↗(On Diff #372965)

Can you add a comment explaining why that's not satisfied on Windows?

This revision is now accepted and ready to land.Oct 6 2021, 12:28 PM
Mordante marked an inline comment as done.Oct 7 2021, 8:15 AM
Mordante added inline comments.
libcxx/test/libcxx/utilities/format/format.string/format.string.std/std_format_spec_char.pass.cpp
437 ↗(On Diff #372965)

I added a TODO FMT. LWG 3576 will probably change the size of the Parser.

This revision was automatically updated to reflect the committed changes.
Mordante marked an inline comment as done.