[libcxx-commits] [PATCH] D93593: [libc++][format] Add __format_arg_store.

[Michael Schellenberger Costa via Phabricator via libcxx-commits]

miscco added a subscriber: cjdb.
miscco added a comment.

Thanks for working on this. It is a considerable library

I am really concerned about doing a "quick" implementation and then being locked in due to ABI issues.

I believe we should spend the time up front to get a clean efficient implementation



================
Comment at: libcxx/include/format:408-418
+  template <class _Tp>
+  _LIBCPP_INLINE_VISIBILITY explicit basic_format_arg(
+      const _Tp& __value) noexcept
+      requires(_VSTD::is_same_v<_Tp, bool> || _VSTD::is_same_v<_Tp, char_type>
+#ifndef _LIBCPP_HAS_NO_INT128
+               || _VSTD::is_same_v<_Tp, __int128_t> ||
+               _VSTD::is_same_v<_Tp, __uint128_t>
----------------
I think we whould really constraint this on `template<integral _Tp>` and handle all integral types here. Having them as multiple separat functions has significant maintainance overhead. Same for `<template<floating_point _Tp>`

I think they are in flight by @cjdb so maybe wait a bit to get this in.

(FYI this i how it I dit it for MSVC STL https://github.com/microsoft/STL/blob/99e8a2d7f2a083a6f768cff35a5e1792feb625a6/stl/inc/format#L933-L962 )


================
Comment at: libcxx/include/format:503-506
+  friend bool operator==(const basic_format_arg& __lhs,
+                         const basic_format_arg& __rhs) {
+    return __lhs.__value_ == __rhs.__value_;
+  }
----------------
I *think* we should be able to default this as it is C++20


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D93593/new/

https://reviews.llvm.org/D93593