From e948cab07d68c240723a12cdc151d09c5cef87ba Mon Sep 17 00:00:00 2001 From: Mark de Wever Date: Tue, 15 Nov 2022 19:53:30 +0100 Subject: [PATCH] [libc++][format] Fixes visit_format_arg. The Standard specifies which types are stored in the basic_format_arg "variant" and which types are stored as a handle. Libc++ stores additional types in the "variant". During a reflector discussion @jwakely mention this is user observable; visit_format_arg uses the type instead of a handle as argument. This optimization is useful and will probably be used for other small types in the future. To be conferment the visitor creates a handle and uses that as argument. There is a second visitor so the formatter can still directly access the 128-bit integrals. The test for the visitor and get has been made public too, there is no reason not too. The 128-bit integral types are required by the Standard, when they are available. Reviewed By: ldionne, #libc Differential Revision: https://reviews.llvm.org/D138052 --- libcxx/include/__format/format_arg.h | 38 ++++++++++++++-- libcxx/include/__format/format_functions.h | 3 +- libcxx/include/__format/parser_std_format_spec.h | 10 +++- .../format.arg/visit_format_arg.pass.cpp | 53 +++++++++------------- .../format.arguments/format.args/get.pass.cpp | 46 +++++++------------ 5 files changed, 82 insertions(+), 68 deletions(-) rename libcxx/test/{libcxx => std}/utilities/format/format.arguments/format.arg/visit_format_arg.pass.cpp (87%) rename libcxx/test/{libcxx => std}/utilities/format/format.arguments/format.args/get.pass.cpp (86%) diff --git a/libcxx/include/__format/format_arg.h b/libcxx/include/__format/format_arg.h index 4f93024..33d931ad 100644 --- a/libcxx/include/__format/format_arg.h +++ b/libcxx/include/__format/format_arg.h @@ -45,16 +45,22 @@ namespace __format { /// It could be packed in 4-bits but that means a new type directly becomes an /// ABI break. The packed type is 64-bit so this reduces the maximum number of /// packed elements from 16 to 12. +/// +/// @note Some members of this enum are an extension. These extensions need +/// special behaviour in visit_format_arg. There they need to be wrapped in a +/// handle to satisfy the user observable behaviour. The internal function +/// __visit_format_arg doesn't do this wrapping. So in the format functions +/// this function is used to avoid unneeded overhead. enum class _LIBCPP_ENUM_VIS __arg_t : uint8_t { __none, __boolean, __char_type, __int, __long_long, - __i128, + __i128, // extension __unsigned, __unsigned_long_long, - __u128, + __u128, // extension __float, __double, __long_double, @@ -85,9 +91,11 @@ constexpr __arg_t __get_packed_type(uint64_t __types, size_t __id) { } // namespace __format +// This function is not user obervable, so it can directly use the non-standard +// types of the "variant". See __arg_t for more details. template -_LIBCPP_HIDE_FROM_ABI _LIBCPP_AVAILABILITY_FORMAT decltype(auto) visit_format_arg(_Visitor&& __vis, - basic_format_arg<_Context> __arg) { +_LIBCPP_HIDE_FROM_ABI _LIBCPP_AVAILABILITY_FORMAT decltype(auto) +__visit_format_arg(_Visitor&& __vis, basic_format_arg<_Context> __arg) { switch (__arg.__type_) { case __format::__arg_t::__none: return _VSTD::invoke(_VSTD::forward<_Visitor>(__vis), __arg.__value_.__monostate_); @@ -265,6 +273,28 @@ private: typename __basic_format_arg_value<_Context>::__handle& __handle_; }; +// This function is user facing, so it must wrap the non-standard types of +// the "variant" in a handle to stay conforming. See __arg_t for more details. +template +_LIBCPP_HIDE_FROM_ABI _LIBCPP_AVAILABILITY_FORMAT decltype(auto) +visit_format_arg(_Visitor&& __vis, basic_format_arg<_Context> __arg) { + switch (__arg.__type_) { +# ifndef _LIBCPP_HAS_NO_INT128 + case __format::__arg_t::__i128: { + typename __basic_format_arg_value<_Context>::__handle __h{__arg.__value_.__i128_}; + return _VSTD::invoke(_VSTD::forward<_Visitor>(__vis), typename basic_format_arg<_Context>::handle{__h}); + } + + case __format::__arg_t::__u128: { + typename __basic_format_arg_value<_Context>::__handle __h{__arg.__value_.__u128_}; + return _VSTD::invoke(_VSTD::forward<_Visitor>(__vis), typename basic_format_arg<_Context>::handle{__h}); + } +# endif + default: + return _VSTD::__visit_format_arg(_VSTD::forward<_Visitor>(__vis), __arg); + } +} + #endif //_LIBCPP_STD_VER > 17 _LIBCPP_END_NAMESPACE_STD diff --git a/libcxx/include/__format/format_functions.h b/libcxx/include/__format/format_functions.h index 2e7925b..8c8b54e 100644 --- a/libcxx/include/__format/format_functions.h +++ b/libcxx/include/__format/format_functions.h @@ -184,6 +184,7 @@ __compile_time_validate_argument(basic_format_parse_context<_CharT>& __parse_ctx __format::__compile_time_validate_integral(__ctx.arg(__formatter.__parser_.__precision_)); } +// This function is not user facing, so it can directly use the non-standard types of the "variant". template _LIBCPP_HIDE_FROM_ABI constexpr void __compile_time_visit_format_arg(basic_format_parse_context<_CharT>& __parse_ctx, __compile_time_basic_format_context<_CharT>& __ctx, @@ -263,7 +264,7 @@ __handle_replacement_field(const _CharT* __begin, const _CharT* __end, else __format::__compile_time_visit_format_arg(__parse_ctx, __ctx, __type); } else - _VSTD::visit_format_arg( + _VSTD::__visit_format_arg( [&](auto __arg) { if constexpr (same_as) __throw_format_error("Argument index out of bounds"); diff --git a/libcxx/include/__format/parser_std_format_spec.h b/libcxx/include/__format/parser_std_format_spec.h index 05f51f7..90c0cd1 100644 --- a/libcxx/include/__format/parser_std_format_spec.h +++ b/libcxx/include/__format/parser_std_format_spec.h @@ -66,7 +66,15 @@ __parse_arg_id(const _CharT* __begin, const _CharT* __end, auto& __parse_ctx) { template _LIBCPP_HIDE_FROM_ABI constexpr uint32_t __substitute_arg_id(basic_format_arg<_Context> __format_arg) { - return visit_format_arg( + // [format.string.std]/8 + // If the corresponding formatting argument is not of integral type... + // This wording allows char and bool too. LWG-3720 changes the wording to + // If the corresponding formatting argument is not of standard signed or + // unsigned integer type, + // This means the 128-bit will not be valid anymore. + // TODO FMT Verify this resolution is accepted and add a test to verify + // 128-bit integrals fail and switch to visit_format_arg. + return _VSTD::__visit_format_arg( [](auto __arg) -> uint32_t { using _Type = decltype(__arg); if constexpr (integral<_Type>) { diff --git a/libcxx/test/libcxx/utilities/format/format.arguments/format.arg/visit_format_arg.pass.cpp b/libcxx/test/std/utilities/format/format.arguments/format.arg/visit_format_arg.pass.cpp similarity index 87% rename from libcxx/test/libcxx/utilities/format/format.arguments/format.arg/visit_format_arg.pass.cpp rename to libcxx/test/std/utilities/format/format.arguments/format.arg/visit_format_arg.pass.cpp index 0f460b6..ea2680a 100644 --- a/libcxx/test/libcxx/utilities/format/format.arguments/format.arg/visit_format_arg.pass.cpp +++ b/libcxx/test/std/utilities/format/format.arguments/format.arg/visit_format_arg.pass.cpp @@ -30,7 +30,7 @@ void test(From value) { auto store = std::make_format_args(value); std::basic_format_args format_args{store}; - assert(format_args.__size() == 1); + LIBCPP_ASSERT(format_args.__size() == 1); assert(format_args.get(0)); auto result = std::visit_format_arg( @@ -49,6 +49,21 @@ void test(From value) { assert(static_cast(result) == static_cast(value)); } +// Some types, as an extension, are stored in the variant. The Standard +// requires them to be observed as a handle. +template +void test_handle(T value) { + auto store = std::make_format_args(value); + std::basic_format_args format_args{store}; + + LIBCPP_ASSERT(format_args.__size() == 1); + assert(format_args.get(0)); + + std::visit_format_arg( + [](auto a) { assert((std::is_same_v::handle>)); }, + format_args.get(0)); +} + // Test specific for string and string_view. // // Since both result in a string_view there's no need to pass this as a @@ -58,7 +73,7 @@ void test_string_view(From value) { auto store = std::make_format_args(value); std::basic_format_args format_args{store}; - assert(format_args.__size() == 1); + LIBCPP_ASSERT(format_args.__size() == 1); assert(format_args.get(0)); using CharT = typename Context::char_type; @@ -183,22 +198,8 @@ void test() { test(std::numeric_limits::max()); #ifndef TEST_HAS_NO_INT128 - test(std::numeric_limits<__int128_t>::min()); - test(std::numeric_limits::min()); - test(std::numeric_limits::min()); - test(std::numeric_limits::min()); - test(std::numeric_limits::min()); - test( - std::numeric_limits::min()); - test(0); - test( - std::numeric_limits::max()); - test(std::numeric_limits::max()); - test(std::numeric_limits::max()); - test(std::numeric_limits::max()); - test(std::numeric_limits::max()); - test(std::numeric_limits<__int128_t>::max()); -#endif + test_handle(0); +#endif // TEST_HAS_NO_INT128 // Test unsigned integer types. @@ -244,20 +245,8 @@ void test() { std::numeric_limits::max()); #ifndef TEST_HAS_NO_INT128 - test(0); - test( - std::numeric_limits::max()); - test( - std::numeric_limits::max()); - test( - std::numeric_limits::max()); - test( - std::numeric_limits::max()); - test( - std::numeric_limits::max()); - test( - std::numeric_limits<__uint128_t>::max()); -#endif + test_handle(0); +#endif // TEST_HAS_NO_INT128 // Test floating point types. diff --git a/libcxx/test/libcxx/utilities/format/format.arguments/format.args/get.pass.cpp b/libcxx/test/std/utilities/format/format.arguments/format.args/get.pass.cpp similarity index 86% rename from libcxx/test/libcxx/utilities/format/format.arguments/format.args/get.pass.cpp rename to libcxx/test/std/utilities/format/format.arguments/format.args/get.pass.cpp index b18c1e3..35bee3ec 100644 --- a/libcxx/test/libcxx/utilities/format/format.arguments/format.args/get.pass.cpp +++ b/libcxx/test/std/utilities/format/format.arguments/format.args/get.pass.cpp @@ -37,6 +37,18 @@ void test(From value) { format_args.get(0)); } +// Some types, as an extension, are stored in the variant. The Standard +// requires them to be observed as a handle. +template +void test_handle(T value) { + auto store = std::make_format_args(value); + std::basic_format_args format_args{store}; + + std::visit_format_arg( + [](auto a) { assert((std::is_same_v::handle>)); }, + format_args.get(0)); +} + // Test specific for string and string_view. // // Since both result in a string_view there's no need to pass this as a @@ -170,22 +182,8 @@ void test() { test(std::numeric_limits::max()); #ifndef TEST_HAS_NO_INT128 - test(std::numeric_limits<__int128_t>::min()); - test(std::numeric_limits::min()); - test(std::numeric_limits::min()); - test(std::numeric_limits::min()); - test(std::numeric_limits::min()); - test( - std::numeric_limits::min()); - test(0); - test( - std::numeric_limits::max()); - test(std::numeric_limits::max()); - test(std::numeric_limits::max()); - test(std::numeric_limits::max()); - test(std::numeric_limits::max()); - test(std::numeric_limits<__int128_t>::max()); -#endif + test_handle(0); +#endif // TEST_HAS_NO_INT128 // Test unsigned integer types. @@ -231,20 +229,8 @@ void test() { std::numeric_limits::max()); #ifndef TEST_HAS_NO_INT128 - test(0); - test( - std::numeric_limits::max()); - test( - std::numeric_limits::max()); - test( - std::numeric_limits::max()); - test( - std::numeric_limits::max()); - test( - std::numeric_limits::max()); - test( - std::numeric_limits<__uint128_t>::max()); -#endif + test_handle(0); +#endif // TEST_HAS_NO_INT128 // Test floating point types. -- 2.7.4