[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

diff --git a/libcxx/include/__format/format_arg.h b/libcxx/include/__format/format_arg.h
index 4f93024..33d931a 100644 (file)
--- 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 @@ 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 <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 2e7925b..8c8b54e 100644 (file)
--- 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 05f51f7..90c0cd1 100644 (file)
--- 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 0f460b6..ea2680a 100644 (file)
--- 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 b18c1e3..35bee3e 100644 (file)
--- 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.