[libc++][format] Fixes visit_format_arg.

[Mordante]

Summary

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.

[ldionne]

libcxx/include/__format/format_arg.h

277–278

282–294

That would reduce duplication below?

libcxx/include/__format/format_functions.h

187

libcxx/include/__format/parser_std_format_spec.h

77

Qualify the call for ADL?

[Mordante]

Thanks for the review!

libcxx/include/__format/format_arg.h

282–294

I was considering something similar, but was doubting whether it would be a good idea.

[Mordante]

Rebased and addresses review comments.

libcxx/include/__format/format_arg.h

 ///
 /// @note The value is stored as a 5-bit value in the __packed_arg_t_bits. This
 /// limits the maximum number of elements to 32.
 /// When modifying update the test
 /// test/libcxx/utilities/format/format.arguments/format.arg/arg_t.compile.pass.cpp
 /// 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,
   __const_char_type_ptr,
   __string_view,
   __ptr,
   __handle
 };
constexpr __arg_t __get_packed_type(uint64_t __types, size_t __id) {
   if (__id > 0)
     __types >>= __id * __packed_arg_t_bits;
 
   return static_cast<__format::__arg_t>(__types & __packed_arg_t_mask);
 }
 
 } // 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_);
   case __format::__arg_t::__boolean:
     return _VSTD::invoke(_VSTD::forward<_Visitor>(__vis), __arg.__value_.__boolean_);
   case __format::__arg_t::__char_type:
     return _VSTD::invoke(_VSTD::forward<_Visitor>(__vis), __arg.__value_.__char_type_);
   case __format::__arg_t::__int:
public:
 
   _LIBCPP_HIDE_FROM_ABI explicit handle(typename __basic_format_arg_value<_Context>::__handle& __handle) noexcept
       : __handle_(__handle) {}
 
 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
 
 #endif // _LIBCPP___FORMAT_FORMAT_ARG_H

libcxx/include/__format/format_functions.h

__compile_time_validate_argument(basic_format_parse_context<_CharT>& __parse_ctx,
   if (__formatter.__parser_.__width_as_arg_)
     __format::__compile_time_validate_integral(__ctx.arg(__formatter.__parser_.__width_));
 
   if constexpr (_HasPrecision)
     if (__formatter.__parser_.__precision_as_arg_)
       __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,
                                                                      __arg_t __type) {
   switch (__type) {
   case __arg_t::__none:
     __throw_format_error("Invalid argument");
   case __arg_t::__boolean:
__handle_replacement_field(const _CharT* __begin, const _CharT* __end,
 
   if constexpr (same_as<_Ctx, __compile_time_basic_format_context<_CharT>>) {
     __arg_t __type = __ctx.arg(__r.__value);
     if (__type == __arg_t::__handle)
       __ctx.__handle(__r.__value).__parse(__parse_ctx);
     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");
           else if constexpr (same_as<decltype(__arg), typename basic_format_arg<_Ctx>::handle>)
             __arg.format(__parse_ctx, __ctx);
           else {
             formatter<decltype(__arg), _CharT> __formatter;
             if (__parse)

libcxx/include/__format/parser_std_format_spec.h

__parse_arg_id(const _CharT* __begin, const _CharT* __end, auto& __parse_ctx) {
 
   ++__r.__ptr;
   return __r;
 }
 
 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>) {
           if constexpr (signed_integral<_Type>) {
             if (__arg < 0)
               __throw_format_error("A format-spec arg-id replacement shouldn't "
                                    "have a negative value");
           }

libcxx/test/std/utilities/format/format.arguments/format.arg/visit_format_arg.pass.cpp

 #include "make_string.h"
 #include "min_allocator.h"
 
 template <class Context, class To, class From>
 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(
       [v = To(value)](auto a) -> To {
         if constexpr (std::is_same_v<To, decltype(a)>) {
           assert(v == a);
           return a;
         } else {
           assert(false);
           return {};
         }
       },
       format_args.get(0));
 
   using ct = std::common_type_t<From, To>;
   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
 // template argument.
 template <class Context, class From>
 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;
   using To = std::basic_string_view<CharT>;
   using V = std::basic_string<CharT>;
   auto result = std::visit_format_arg(
       [v = V(value.begin(), value.end())](auto a) -> To {
         if constexpr (std::is_same_v<To, decltype(a)>) {
void test() {
   test<Context, long long, long long>(0);
   test<Context, long long, long long>(std::numeric_limits<signed char>::max());
   test<Context, long long, long long>(std::numeric_limits<short>::max());
   test<Context, long long, long long>(std::numeric_limits<int>::max());
   test<Context, long long, long long>(std::numeric_limits<long>::max());
   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.
 
   test<Context, unsigned, unsigned char>(0);
   test<Context, unsigned, unsigned char>(
       std::numeric_limits<unsigned char>::max());
 
   test<Context, unsigned, unsigned short>(0);
#endif // TEST_HAS_NO_INT128
   test<Context, unsigned long long, unsigned long long>(
       std::numeric_limits<unsigned>::max());
   test<Context, unsigned long long, unsigned long long>(
       std::numeric_limits<unsigned long>::max());
   test<Context, unsigned long long, unsigned long long>(
       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.
 
   test<Context, float, float>(-std::numeric_limits<float>::max());
   test<Context, float, float>(-std::numeric_limits<float>::min());
   test<Context, float, float>(-0.0);
   test<Context, float, float>(0.0);
   test<Context, float, float>(std::numeric_limits<float>::min());

libcxx/test/std/utilities/format/format.arguments/format.args/get.pass.cpp

std::visit_format_arg(
         if constexpr (std::is_same_v<To, decltype(a)>)
           assert(v == a);
         else
           assert(false);
       },
       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
 // template argument.
 template <class Context, class From>
 void test_string_view(From value) {
   auto store = std::make_format_args<Context>(value);
   const std::basic_format_args<Context> format_args{store};
void test() {
   test<Context, long long, long long>(0);
   test<Context, long long, long long>(std::numeric_limits<signed char>::max());
   test<Context, long long, long long>(std::numeric_limits<short>::max());
   test<Context, long long, long long>(std::numeric_limits<int>::max());
   test<Context, long long, long long>(std::numeric_limits<long>::max());
   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.
 
   test<Context, unsigned, unsigned char>(0);
   test<Context, unsigned, unsigned char>(
       std::numeric_limits<unsigned char>::max());
 
   test<Context, unsigned, unsigned short>(0);
#endif // TEST_HAS_NO_INT128
   test<Context, unsigned long long, unsigned long long>(
       std::numeric_limits<unsigned>::max());
   test<Context, unsigned long long, unsigned long long>(
       std::numeric_limits<unsigned long>::max());
   test<Context, unsigned long long, unsigned long long>(
       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.
 
   test<Context, float, float>(-std::numeric_limits<float>::max());
   test<Context, float, float>(-std::numeric_limits<float>::min());
   test<Context, float, float>(-0.0);
   test<Context, float, float>(0.0);
   test<Context, float, float>(std::numeric_limits<float>::min());