Page MenuHomePhabricator

[libc++][format] Implement formatters.
Needs RevisionPublic

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

Details

Reviewers
EricWF
ldionne
mclow.lists
miscco
curdeius
vitaut
Group Reviewers
Restricted Project
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

Unit TestsFailed

TimeTest
530 mslibcxx CI GCC-next/C++20 > libc++.std/atomics/atomics_types_operations/atomics_types_operations_req::atomic_is_lock_free.pass.cpp
Script: -- : 'COMPILED WITH'; /usr/bin/g++-11 /home/libcxx-builder/.buildkite-agent/builds/021b21117897-1/llvm-project/libcxx-ci/libcxx/test/std/atomics/atomics.types.operations/atomics.types.operations.req/atomic_is_lock_free.pass.cpp -v -include /home/libcxx-builder/.buildkite-agent/builds/021b21117897-1/llvm-project/libcxx-ci/libcxx/test/support/nasty_macros.h -nostdinc++ -I/home/libcxx-builder/.buildkite-agent/builds/021b21117897-1/llvm-project/libcxx-ci/build/generic-gcc-next/include/c++/v1 -I/home/libcxx-builder/.buildkite-agent/builds/021b21117897-1/llvm-project/libcxx-ci/build/generic-gcc-next/projects/libcxx/include/c++build -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -D__STDC_CONSTANT_MACROS -I/home/libcxx-builder/.buildkite-agent/builds/021b21117897-1/llvm-project/libcxx-ci/libcxx/test/support -std=c++2b -Werror -Wall -Wextra -Wshadow -Wundef -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-user-defined-literals -Wno-noexcept-type -Wno-aligned-allocation-unavailable -Wno-atomic-alignment -Wno-sized-deallocation -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -D_LIBCPP_DISABLE_AVAILABILITY -Wno-macro-redefined -D_LIBCPP_HAS_THREAD_API_PTHREAD -Wno-macro-redefined -D_LIBCPP_ABI_VERSION=1 -lc++experimental -L/home/libcxx-builder/.buildkite-agent/builds/021b21117897-1/llvm-project/libcxx-ci/build/generic-gcc-next/./lib -Wl,-rpath,/home/libcxx-builder/.buildkite-agent/builds/021b21117897-1/llvm-project/libcxx-ci/build/generic-gcc-next/./lib -L/home/libcxx-builder/.buildkite-agent/builds/021b21117897-1/llvm-project/libcxx-ci/build/generic-gcc-next/./lib -Wl,-rpath,/home/libcxx-builder/.buildkite-agent/builds/021b21117897-1/llvm-project/libcxx-ci/build/generic-gcc-next/./lib -nodefaultlibs -lc++ -lm -lgcc_s -lgcc -lpthread -lc -lgcc_s -lgcc -latomic -o /home/libcxx-builder/.buildkite-agent/builds/021b21117897-1/llvm-project/libcxx-ci/build/generic-gcc-next/projects/libcxx/test/std/atomics/atomics.types.
1,590 mslibcxx CI GCC-next/C++20 > libc++.std/iterators/iterator_primitives/range_iter_ops/range_iter_ops_advance::advance.pass.cpp
Script: -- : 'COMPILED WITH'; /usr/bin/g++-11 /home/libcxx-builder/.buildkite-agent/builds/021b21117897-1/llvm-project/libcxx-ci/libcxx/test/std/iterators/iterator.primitives/range.iter.ops/range.iter.ops.advance/advance.pass.cpp -v -include /home/libcxx-builder/.buildkite-agent/builds/021b21117897-1/llvm-project/libcxx-ci/libcxx/test/support/nasty_macros.h -nostdinc++ -I/home/libcxx-builder/.buildkite-agent/builds/021b21117897-1/llvm-project/libcxx-ci/build/generic-gcc-next/include/c++/v1 -I/home/libcxx-builder/.buildkite-agent/builds/021b21117897-1/llvm-project/libcxx-ci/build/generic-gcc-next/projects/libcxx/include/c++build -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -D__STDC_CONSTANT_MACROS -I/home/libcxx-builder/.buildkite-agent/builds/021b21117897-1/llvm-project/libcxx-ci/libcxx/test/support -std=c++2b -Werror -Wall -Wextra -Wshadow -Wundef -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-user-defined-literals -Wno-noexcept-type -Wno-aligned-allocation-unavailable -Wno-atomic-alignment -Wno-sized-deallocation -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -D_LIBCPP_DISABLE_AVAILABILITY -Wno-macro-redefined -D_LIBCPP_HAS_THREAD_API_PTHREAD -Wno-macro-redefined -D_LIBCPP_ABI_VERSION=1 -lc++experimental -L/home/libcxx-builder/.buildkite-agent/builds/021b21117897-1/llvm-project/libcxx-ci/build/generic-gcc-next/./lib -Wl,-rpath,/home/libcxx-builder/.buildkite-agent/builds/021b21117897-1/llvm-project/libcxx-ci/build/generic-gcc-next/./lib -L/home/libcxx-builder/.buildkite-agent/builds/021b21117897-1/llvm-project/libcxx-ci/build/generic-gcc-next/./lib -Wl,-rpath,/home/libcxx-builder/.buildkite-agent/builds/021b21117897-1/llvm-project/libcxx-ci/build/generic-gcc-next/./lib -nodefaultlibs -lc++ -lm -lgcc_s -lgcc -lpthread -lc -lgcc_s -lgcc -latomic -o /home/libcxx-builder/.buildkite-agent/builds/021b21117897-1/llvm-project/libcxx-ci/build/generic-gcc-next/projects/libcxx/test/std/iterators/iterator.
1,230 mslibcxx CI GCC-next/C++20 > libc++.std/iterators/iterator_primitives/range_iter_ops/range_iter_ops_next::iterator_count.pass.cpp
Script: -- : 'COMPILED WITH'; /usr/bin/g++-11 /home/libcxx-builder/.buildkite-agent/builds/021b21117897-1/llvm-project/libcxx-ci/libcxx/test/std/iterators/iterator.primitives/range.iter.ops/range.iter.ops.next/iterator_count.pass.cpp -v -include /home/libcxx-builder/.buildkite-agent/builds/021b21117897-1/llvm-project/libcxx-ci/libcxx/test/support/nasty_macros.h -nostdinc++ -I/home/libcxx-builder/.buildkite-agent/builds/021b21117897-1/llvm-project/libcxx-ci/build/generic-gcc-next/include/c++/v1 -I/home/libcxx-builder/.buildkite-agent/builds/021b21117897-1/llvm-project/libcxx-ci/build/generic-gcc-next/projects/libcxx/include/c++build -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -D__STDC_CONSTANT_MACROS -I/home/libcxx-builder/.buildkite-agent/builds/021b21117897-1/llvm-project/libcxx-ci/libcxx/test/support -std=c++2b -Werror -Wall -Wextra -Wshadow -Wundef -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-user-defined-literals -Wno-noexcept-type -Wno-aligned-allocation-unavailable -Wno-atomic-alignment -Wno-sized-deallocation -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -D_LIBCPP_DISABLE_AVAILABILITY -Wno-macro-redefined -D_LIBCPP_HAS_THREAD_API_PTHREAD -Wno-macro-redefined -D_LIBCPP_ABI_VERSION=1 -lc++experimental -L/home/libcxx-builder/.buildkite-agent/builds/021b21117897-1/llvm-project/libcxx-ci/build/generic-gcc-next/./lib -Wl,-rpath,/home/libcxx-builder/.buildkite-agent/builds/021b21117897-1/llvm-project/libcxx-ci/build/generic-gcc-next/./lib -L/home/libcxx-builder/.buildkite-agent/builds/021b21117897-1/llvm-project/libcxx-ci/build/generic-gcc-next/./lib -Wl,-rpath,/home/libcxx-builder/.buildkite-agent/builds/021b21117897-1/llvm-project/libcxx-ci/build/generic-gcc-next/./lib -nodefaultlibs -lc++ -lm -lgcc_s -lgcc -lpthread -lc -lgcc_s -lgcc -latomic -o /home/libcxx-builder/.buildkite-agent/builds/021b21117897-1/llvm-project/libcxx-ci/build/generic-gcc-next/projects/libcxx/test/std/iterators/iterator.
1,230 mslibcxx CI GCC-next/C++20 > libc++.std/iterators/iterator_primitives/range_iter_ops/range_iter_ops_next::iterator_count_sentinel.pass.cpp
Script: -- : 'COMPILED WITH'; /usr/bin/g++-11 /home/libcxx-builder/.buildkite-agent/builds/021b21117897-1/llvm-project/libcxx-ci/libcxx/test/std/iterators/iterator.primitives/range.iter.ops/range.iter.ops.next/iterator_count_sentinel.pass.cpp -v -include /home/libcxx-builder/.buildkite-agent/builds/021b21117897-1/llvm-project/libcxx-ci/libcxx/test/support/nasty_macros.h -nostdinc++ -I/home/libcxx-builder/.buildkite-agent/builds/021b21117897-1/llvm-project/libcxx-ci/build/generic-gcc-next/include/c++/v1 -I/home/libcxx-builder/.buildkite-agent/builds/021b21117897-1/llvm-project/libcxx-ci/build/generic-gcc-next/projects/libcxx/include/c++build -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -D__STDC_CONSTANT_MACROS -I/home/libcxx-builder/.buildkite-agent/builds/021b21117897-1/llvm-project/libcxx-ci/libcxx/test/support -std=c++2b -Werror -Wall -Wextra -Wshadow -Wundef -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-user-defined-literals -Wno-noexcept-type -Wno-aligned-allocation-unavailable -Wno-atomic-alignment -Wno-sized-deallocation -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -D_LIBCPP_DISABLE_AVAILABILITY -Wno-macro-redefined -D_LIBCPP_HAS_THREAD_API_PTHREAD -Wno-macro-redefined -D_LIBCPP_ABI_VERSION=1 -lc++experimental -L/home/libcxx-builder/.buildkite-agent/builds/021b21117897-1/llvm-project/libcxx-ci/build/generic-gcc-next/./lib -Wl,-rpath,/home/libcxx-builder/.buildkite-agent/builds/021b21117897-1/llvm-project/libcxx-ci/build/generic-gcc-next/./lib -L/home/libcxx-builder/.buildkite-agent/builds/021b21117897-1/llvm-project/libcxx-ci/build/generic-gcc-next/./lib -Wl,-rpath,/home/libcxx-builder/.buildkite-agent/builds/021b21117897-1/llvm-project/libcxx-ci/build/generic-gcc-next/./lib -nodefaultlibs -lc++ -lm -lgcc_s -lgcc -lpthread -lc -lgcc_s -lgcc -latomic -o /home/libcxx-builder/.buildkite-agent/builds/021b21117897-1/llvm-project/libcxx-ci/build/generic-gcc-next/projects/libcxx/test/std/iterators/iterator.
1,450 mslibcxx CI GCC-next/C++20 > libc++.std/iterators/iterator_primitives/range_iter_ops/range_iter_ops_next::iterator_sentinel.pass.cpp
Script: -- : 'COMPILED WITH'; /usr/bin/g++-11 /home/libcxx-builder/.buildkite-agent/builds/021b21117897-1/llvm-project/libcxx-ci/libcxx/test/std/iterators/iterator.primitives/range.iter.ops/range.iter.ops.next/iterator_sentinel.pass.cpp -v -include /home/libcxx-builder/.buildkite-agent/builds/021b21117897-1/llvm-project/libcxx-ci/libcxx/test/support/nasty_macros.h -nostdinc++ -I/home/libcxx-builder/.buildkite-agent/builds/021b21117897-1/llvm-project/libcxx-ci/build/generic-gcc-next/include/c++/v1 -I/home/libcxx-builder/.buildkite-agent/builds/021b21117897-1/llvm-project/libcxx-ci/build/generic-gcc-next/projects/libcxx/include/c++build -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -D__STDC_CONSTANT_MACROS -I/home/libcxx-builder/.buildkite-agent/builds/021b21117897-1/llvm-project/libcxx-ci/libcxx/test/support -std=c++2b -Werror -Wall -Wextra -Wshadow -Wundef -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-user-defined-literals -Wno-noexcept-type -Wno-aligned-allocation-unavailable -Wno-atomic-alignment -Wno-sized-deallocation -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -D_LIBCPP_DISABLE_AVAILABILITY -Wno-macro-redefined -D_LIBCPP_HAS_THREAD_API_PTHREAD -Wno-macro-redefined -D_LIBCPP_ABI_VERSION=1 -lc++experimental -L/home/libcxx-builder/.buildkite-agent/builds/021b21117897-1/llvm-project/libcxx-ci/build/generic-gcc-next/./lib -Wl,-rpath,/home/libcxx-builder/.buildkite-agent/builds/021b21117897-1/llvm-project/libcxx-ci/build/generic-gcc-next/./lib -L/home/libcxx-builder/.buildkite-agent/builds/021b21117897-1/llvm-project/libcxx-ci/build/generic-gcc-next/./lib -Wl,-rpath,/home/libcxx-builder/.buildkite-agent/builds/021b21117897-1/llvm-project/libcxx-ci/build/generic-gcc-next/./lib -nodefaultlibs -lc++ -lm -lgcc_s -lgcc -lpthread -lc -lgcc_s -lgcc -latomic -o /home/libcxx-builder/.buildkite-agent/builds/021b21117897-1/llvm-project/libcxx-ci/build/generic-gcc-next/projects/libcxx/test/std/iterators/iterator.
View Full Test Results (12 Failed)

Event Timeline

Mordante created this revision.Feb 14 2021, 6:01 AM
Mordante requested review of this revision.Feb 14 2021, 6:01 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 14 2021, 6:01 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Mordante updated this revision to Diff 323617.Feb 14 2021, 7:13 AM

Fixes a minor formatting error.
2 unit test with a last minute cleanup breakage
a unit test requiring clang-12 due since it requires e97e9851b227e
properly silence warnings about unused parameter pack arguments

vitaut added a subscriber: vitaut.Apr 22 2021, 11:15 AM
Mordante updated this revision to Diff 348629.Sat, May 29, 5:26 AM

Reworked the enitre patch.
Changed the dependency from D93593 to D103357 since the former no longer can be build on the CI.
There are still some minor changes planned, but want to test the CI build.

Mordante updated this revision to Diff 348633.Sat, May 29, 8:15 AM

Fix CI issues:

  • Remove an extra semi-column
  • Remove clang::musttail attribute since not everywhere supported
  • Disable debug iterators for a test copying an library string
Quuxplusone added inline comments.
libcxx/include/format
440–442

Yes! Thank you! Can someone reply to my May 1 email to libcxx-dev about this? :P
I just don't see how debug iterators are intended to ever work in practice.

Mordante updated this revision to Diff 348638.Sat, May 29, 9:27 AM

Another attempt to fix the build.

  • used #ifndef instead of #ifdef
  • test whether the CI accepts Arthurs UNSUPPORTED helper
Mordante marked an inline comment as done.Sat, May 29, 9:29 AM
Mordante added inline comments.
libcxx/include/format
440–442

Yeah I forgot about this work-around I added quite a while ago.

Mordante updated this revision to Diff 348652.Sat, May 29, 11:01 AM
Mordante marked an inline comment as done.

Disable the iostream in unit tests when localization is disabled. This should fix the no localization build.

Mordante updated this revision to Diff 348679.Sun, May 30, 3:09 AM

Disable the formatter.const_char_array.pass.cpp on gcc 11 and apple-clang.

Mordante updated this revision to Diff 349177.Tue, Jun 1, 10:43 PM

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)Sat, Jun 5, 4:07 AM
Mordante added a reviewer: vitaut.
Mordante edited the summary of this revision. (Show Details)Wed, Jun 9, 8:54 AM
vitaut requested changes to this revision.Sun, Jun 13, 8:22 AM

Overall looks good, some comments inline.

libcxx/include/__format/format_string.h
38

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.

67

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.

91–93

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.

122

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

140

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
34

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

libcxx/include/format
69

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.

351

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

618

typo: sequence

632

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

645

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

779

What does PoC stand for?

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

party -> partly throughout the diff

128–130

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.Sun, Jun 13, 8:22 AM
vitaut added inline comments.Sun, Jun 13, 8:45 AM
libcxx/include/__format/format_string.h
111–113

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

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

65–66

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

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