This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by Mordante on May 31 2021, 12:32 PM.

Details

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

Implements the formatter for all string types.
[format.formatter.spec]/2.2
For each charT, the string type specializations

template<> struct formatter<charT*, charT>;
template<> struct formatter<const charT*, charT>;
template<size_t N> struct formatter<const charT[N], charT>;
template<class traits, class Allocator>
  struct formatter<basic_string<charT, traits, Allocator>, charT>;
template<class traits>
  struct formatter<basic_string_view<charT, traits>, charT>;

This removes the stub implemented in D96664.

Implements parts of:

  • P0645 Text Formatting
  • P1868 width: clarifying units of width and precision in std::format

Diff Detail

Event Timeline

Mordante created this revision.May 31 2021, 12:32 PM
Mordante requested review of this revision.May 31 2021, 12:32 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 31 2021, 12:32 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Mordante updated this revision to Diff 349461.Jun 2 2021, 11:25 PM
  • Rebase
  • Rename __pre and __post since they are reserved on Windows. D103541 will add these names to the nasty macros
  • Remove a dupliated hunk. The hunk was moved to its own function to used for Boolean formatting, but the original remained instead of calling the new function.
Mordante updated this revision to Diff 349574.Jun 3 2021, 9:05 AM

Disable the failing part of a test on Windows.

Mordante edited the summary of this revision. (Show Details)Jun 5 2021, 4:15 AM
Mordante added reviewers: curdeius, ldionne, miscco, vitaut.
vitaut requested changes to this revision.Jun 20 2021, 7:41 AM
vitaut added inline comments.
libcxx/include/CMakeLists.txt
112

Not sure if it's worth having such a fine-grained header split for built-in formatters considering that all of them are pretty small and end up being included in <format>.

libcxx/include/__format/formatter.h
64

As in earlier diffs I suggest using a struct instead of pair.

Also inline seems redundant.

102

Is the space between < and = intentional?

110

Something is missing after "For Unicode this".

116–117

This function returns an iterator, not a char type.

129

Fill can consist of multiple code units.

142

As commented elsewhere __number_max as currently defined is a valid precision and shouldn't be used for "no limit".

libcxx/include/__format/formatter_string.h
40

This class looks unnecessary, why not fold it into __formatter_string?

88–89

Width being greater than precision is perfectly valid and useful. It shouldn't result in an error.

This revision now requires changes to proceed.Jun 20 2021, 7:41 AM
Mordante marked 9 inline comments as done.Jun 29 2021, 10:20 AM
Mordante added inline comments.
libcxx/include/CMakeLists.txt
112

Recently libc++ started to move towards using tiny headers. This started with the people working on ranges and I like that direction. So that's why I made these small headers. (This also makes it a lot easier to maintain larger patch series.)

libcxx/include/__format/formatter.h
116–117

This returns refers the the to_char function, which should have been std::to_chars. I added a @returns and improved this @note to avoid confusion.
This integer part isn't added yet, but is part of D103433.

129

For now libc++ uses one character as mentioned in D103368. I've added a comment as reminder that this needs to be adjusted in the future.

142

The new invalid value is -1.

libcxx/include/__format/formatter_string.h
40

In my original design it seemed nice, but now it's indeed unnecessary.

88–89

Interesting. I overlooked that in the Standard. I've adjusted to code to support this case and added a test.

Mordante updated this revision to Diff 355311.Jun 29 2021, 11:31 AM
Mordante marked 6 inline comments as done.
Mordante edited the summary of this revision. (Show Details)

Addresses review comments and other changes:

  • remove std::pair
  • improve comments
  • change the value of no precision -1 instead of __number_max
  • merge the class __parser_string with __formatter_string
  • allow precision < width as required by the Standard
  • update the module map
  • integrate changes done before in the patch series
Mordante updated this revision to Diff 357283.Jul 8 2021, 10:58 AM

Rebase to get all changes in and resolve merge conflicts.

vitaut accepted this revision.Jul 17 2021, 7:48 AM

Some nits inline, otherwise LGTM.

libcxx/include/__format/formatter.h
124–126

Did you mean the type of values in [__first, __last) being different from the type of __fill? __first and __last themselves are pointers, not code units.

Also the comment says "This function returns a buffer with @c char elements." while in fact it returns an output iterator.

libcxx/include/__format/formatter_string.h
102

nit: public is redundant here and elsewhere since this is a struct.

Mordante marked 2 inline comments as done.Jul 18 2021, 7:05 AM
Mordante added inline comments.
libcxx/include/__format/formatter.h
124–126

I updated the wording.

libcxx/include/__format/formatter_string.h
102

true, I'm just used to writing this explicitly. AFAIK that's also the code convention in libc++. So for now I keep it unless @ldionne objects.

Mordante updated this revision to Diff 359626.Jul 18 2021, 7:16 AM
Mordante marked 2 inline comments as done.

Rebase
Contrain the formatter to the allowed character types
Remove the monolithic algorithm include
Use private module headers
Guard unit tests for implementation details
Addresses review comments

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 364358.Aug 4 2021, 10:53 PM

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

Mordante updated this revision to Diff 370729.Sep 4 2021, 4:38 AM

Rebased and moved tests from the old .inc to the new .h file.

Mordante updated this revision to Diff 372754.Sep 15 2021, 11:12 AM

Remove the unneeded _LIBCPP_PUSH_MACROS.

ldionne accepted this revision.Oct 6 2021, 11:57 AM
ldionne added inline comments.
libcxx/include/__format/formatter_string.h
102

I don't think we have a convention for this. No strong opinion.

This revision is now accepted and ready to land.Oct 6 2021, 11:57 AM
This revision was automatically updated to reflect the committed changes.