[libcxx-commits] [libcxx] e948cab - [libc++][format] Fixes visit_format_arg.
Mark de Wever via libcxx-commits
libcxx-commits at lists.llvm.org
Tue Nov 22 08:48:39 PST 2022
Author: Mark de Wever
Date: 2022-11-22T17:48:33+01:00
New Revision: e948cab07d68c240723a12cdc151d09c5cef87ba
URL: https://github.com/llvm/llvm-project/commit/e948cab07d68c240723a12cdc151d09c5cef87ba
DIFF: https://github.com/llvm/llvm-project/commit/e948cab07d68c240723a12cdc151d09c5cef87ba.diff
LOG: [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
Added:
libcxx/test/std/utilities/format/format.arguments/format.arg/visit_format_arg.pass.cpp
libcxx/test/std/utilities/format/format.arguments/format.args/get.pass.cpp
Modified:
libcxx/include/__format/format_arg.h
libcxx/include/__format/format_functions.h
libcxx/include/__format/parser_std_format_spec.h
Removed:
libcxx/test/libcxx/utilities/format/format.arguments/format.arg/visit_format_arg.pass.cpp
libcxx/test/libcxx/utilities/format/format.arguments/format.args/get.pass.cpp
################################################################################
diff --git a/libcxx/include/__format/format_arg.h b/libcxx/include/__format/format_arg.h
index 4f93024b7c697..33d931ad797fe 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 <class _Visitor, class _Context>
-_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 @@ class _LIBCPP_TEMPLATE_VIS basic_format_arg<_Context>::handle {
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 <class _Visitor, class _Context>
+_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 2e7925bfbd01e..8c8b54e80818a 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 <class _CharT>
_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<decltype(__arg), monostate>)
__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 05f51f7cf9b94..90c0cd184ce1f 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 <class _Context>
_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 0f460b69d167a..ea2680a988bf2 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<Context>(value);
std::basic_format_args<Context> 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<ct>(result) == static_cast<ct>(value));
}
+// Some types, as an extension, are stored in the variant. The Standard
+// requires them to be observed as a handle.
+template <class Context, class T>
+void test_handle(T value) {
+ auto store = std::make_format_args<Context>(value);
+ std::basic_format_args<Context> 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<decltype(a), typename std::basic_format_arg<Context>::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<Context>(value);
std::basic_format_args<Context> 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<Context, long long, long long>(std::numeric_limits<long long>::max());
#ifndef TEST_HAS_NO_INT128
- test<Context, __int128_t, __int128_t>(std::numeric_limits<__int128_t>::min());
- test<Context, __int128_t, __int128_t>(std::numeric_limits<long long>::min());
- test<Context, __int128_t, __int128_t>(std::numeric_limits<long>::min());
- test<Context, __int128_t, __int128_t>(std::numeric_limits<int>::min());
- test<Context, __int128_t, __int128_t>(std::numeric_limits<short>::min());
- test<Context, __int128_t, __int128_t>(
- std::numeric_limits<signed char>::min());
- test<Context, __int128_t, __int128_t>(0);
- test<Context, __int128_t, __int128_t>(
- std::numeric_limits<signed char>::max());
- test<Context, __int128_t, __int128_t>(std::numeric_limits<short>::max());
- test<Context, __int128_t, __int128_t>(std::numeric_limits<int>::max());
- test<Context, __int128_t, __int128_t>(std::numeric_limits<long>::max());
- test<Context, __int128_t, __int128_t>(std::numeric_limits<long long>::max());
- test<Context, __int128_t, __int128_t>(std::numeric_limits<__int128_t>::max());
-#endif
+ test_handle<Context, __int128_t>(0);
+#endif // TEST_HAS_NO_INT128
// Test unsigned integer types.
@@ -244,20 +245,8 @@ void test() {
std::numeric_limits<unsigned long long>::max());
#ifndef TEST_HAS_NO_INT128
- test<Context, __uint128_t, __uint128_t>(0);
- test<Context, __uint128_t, __uint128_t>(
- std::numeric_limits<unsigned char>::max());
- test<Context, __uint128_t, __uint128_t>(
- std::numeric_limits<unsigned short>::max());
- test<Context, __uint128_t, __uint128_t>(
- std::numeric_limits<unsigned int>::max());
- test<Context, __uint128_t, __uint128_t>(
- std::numeric_limits<unsigned long>::max());
- test<Context, __uint128_t, __uint128_t>(
- std::numeric_limits<unsigned long long>::max());
- test<Context, __uint128_t, __uint128_t>(
- std::numeric_limits<__uint128_t>::max());
-#endif
+ test_handle<Context, __uint128_t>(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 b18c1e3233ae5..35bee3ecce59c 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 <class Context, class T>
+void test_handle(T value) {
+ auto store = std::make_format_args<Context>(value);
+ std::basic_format_args<Context> format_args{store};
+
+ std::visit_format_arg(
+ [](auto a) { assert((std::is_same_v<decltype(a), typename std::basic_format_arg<Context>::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<Context, long long, long long>(std::numeric_limits<long long>::max());
#ifndef TEST_HAS_NO_INT128
- test<Context, __int128_t, __int128_t>(std::numeric_limits<__int128_t>::min());
- test<Context, __int128_t, __int128_t>(std::numeric_limits<long long>::min());
- test<Context, __int128_t, __int128_t>(std::numeric_limits<long>::min());
- test<Context, __int128_t, __int128_t>(std::numeric_limits<int>::min());
- test<Context, __int128_t, __int128_t>(std::numeric_limits<short>::min());
- test<Context, __int128_t, __int128_t>(
- std::numeric_limits<signed char>::min());
- test<Context, __int128_t, __int128_t>(0);
- test<Context, __int128_t, __int128_t>(
- std::numeric_limits<signed char>::max());
- test<Context, __int128_t, __int128_t>(std::numeric_limits<short>::max());
- test<Context, __int128_t, __int128_t>(std::numeric_limits<int>::max());
- test<Context, __int128_t, __int128_t>(std::numeric_limits<long>::max());
- test<Context, __int128_t, __int128_t>(std::numeric_limits<long long>::max());
- test<Context, __int128_t, __int128_t>(std::numeric_limits<__int128_t>::max());
-#endif
+ test_handle<Context, __int128_t>(0);
+#endif // TEST_HAS_NO_INT128
// Test unsigned integer types.
@@ -231,20 +229,8 @@ void test() {
std::numeric_limits<unsigned long long>::max());
#ifndef TEST_HAS_NO_INT128
- test<Context, __uint128_t, __uint128_t>(0);
- test<Context, __uint128_t, __uint128_t>(
- std::numeric_limits<unsigned char>::max());
- test<Context, __uint128_t, __uint128_t>(
- std::numeric_limits<unsigned short>::max());
- test<Context, __uint128_t, __uint128_t>(
- std::numeric_limits<unsigned int>::max());
- test<Context, __uint128_t, __uint128_t>(
- std::numeric_limits<unsigned long>::max());
- test<Context, __uint128_t, __uint128_t>(
- std::numeric_limits<unsigned long long>::max());
- test<Context, __uint128_t, __uint128_t>(
- std::numeric_limits<__uint128_t>::max());
-#endif
+ test_handle<Context, __uint128_t>(0);
+#endif // TEST_HAS_NO_INT128
// Test floating point types.
More information about the libcxx-commits
mailing list