[libcxx-commits] [PATCH] D116120: [libc++][format] Improve ABI stability.

[Mark de Wever via Phabricator via libcxx-commits]

Mordante marked 2 inline comments as done.
Mordante added inline comments.


================
Comment at: libcxx/include/__format/format_arg.h:42-43
+///
+/// @note The 128-bit types are unconditionally in the list to avoid the values
+/// of the enums to depend on the availability of 128-bit integers.
 enum class _LIBCPP_ENUM_VIS __arg_t : uint8_t {
----------------
Quuxplusone wrote:
> Consider replacing this comment with a test in `libcxx/test/libcxx/` that does `assert(__format::__arg_t::__ptr == 14);` and is //not// skipped when `_LIBCPP_HAS_NO_INT128`.
> Mantra: "If you liked it then you should have put a test on it!" If we're trying to ensure the ABI doesn't break here, let's put a test on that ABI.
I don't want to remove the comment, but I like your idea of adding a test.


================
Comment at: libcxx/include/__format/format_arg.h:96-105
+  // When _LIBCPP_HAS_NO_INT128 is defined these cases fallthrough to
+  // _LIBCPP_UNREACHABLE().
+  case __format::__arg_t::__i128:
+#ifndef _LIBCPP_HAS_NO_INT128
+    return _VSTD::invoke(_VSTD::forward<_Visitor>(__vis), __arg.__i128);
+#endif
+  case __format::__arg_t::__u128:
----------------
Quuxplusone wrote:
> Murphy's Law says that somebody //will// ignore this comment and add a `case somethingelse:` on line 106, at which point the comment will be wrong because these cases will fallthrough to //that//.
> I guess they'll fall through only when int128 is unsupported and so we shouldn't be hitting these cases anyway... but still, can you just make both cases explicitly do `#else _LIBCPP_UNREACHABLE() #endif` instead?
I was doubting between this solution and the one you suggested. This convinces me the other solution is better.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116120