[libc++][ABI Break] Make is_error_condition_enum_v and is_error_code_enum_v bool, not size_t

[jloser]

Summary

is_error_condition_enum_v and is_error_code_enum_v are currently of
type size_t, but the standard mandates they are of type bool.

This is an ABI break technically since the size of these variable
templates has changed. Document it as such in the release notes.

Fixes https://bugs.llvm.org/show_bug.cgi?id=50755

[• Quuxplusone]

Arguably this should add a regression test. Certainly seems straightforward enough, though, right?
As usual I'm a little unsure of what the ABI implications are (notice is_error_code_enum_v is not _LIBCPP_HIDE_FROM_ABI), but this is also clearly a bugfix and I assume we'd bite the bullet if there were one.

[jloser]

Add tests to verify the variable templates are of the right type

[jloser]

In D112553#3087946, @Quuxplusone wrote:

Arguably this should add a regression test. Certainly seems straightforward enough, though, right?
As usual I'm a little unsure of what the ABI implications are (notice is_error_code_enum_v is not _LIBCPP_HIDE_FROM_ABI), but this is also clearly a bugfix and I assume we'd bite the bullet if there were one.

I was on the fence given how "trivially obvious" the change is, but looks like we did get it wrong at one point...so, I added a simple test for both variable templates.

I agree regarding the ABI break this technically is, but, like you said, it's a bugfix, so it is what it is. I recall in one of my recent PRs we talked about symbol visibility wrt variable templates, but don't think any of us have an immediate planned follow-up. Louis and I have talked offline briefly about some ideas of testing for symbol visibility before we go change the library as a whole in one sweeping fashion.

@ldionne - should we make these variable templates as _LIBCPP_HIDE_FROM_ABI while we're here?

[Mordante]

Changing the size of the object sounds like an ABI break to me. So I'd like @ldionne to decide whether this change requires an ABI version guard or not. Other then that the change LGTM.

[var-const]

LGTM (once the ABI discussion is resolved).

[ldionne]

It is technically an ABI break, but I literally can't imagine someone being bitten by it -- perhaps I'm not imaginative enough? One would have to basically have a data member or function parameter of type decltype(is_error_condition_enum_v<...>), or something along those lines. Given the name of the utility, which starts with is_, everybody has probably been using it correctly, i.e. as a compile-time boolean and nothing else. IMO not fixing that bug based on ABI break concerns would be unreasonable. I'll take the responsibility for it if things turn sour :-). Let's add a release note and call it a day.

Regarding adding visibility/linkage annotations to those variable templates -- I wouldn't do it, I think we'll want to find a solution to treat all the variable templates consistently.

[jloser]

Add release note detailing the ABI break

[ldionne]

Ship it! Our CI is unstable right now. Given the nature and triviality of this change, I think it's reasonable to ship this if your tests pass locally.

libcxx/docs/ReleaseNotes.rst

- The call of the functions ``std::atomic_(add|sub)(std::atomic<T*>*, ...)``
   compiler are unaffected by this change.
 
 - The functions ``std::allocator<T>::allocate`` and
   ``std::experimental::pmr::polymorphic_allocator<T>::allocate`` now throw
   an exception of type ``std::bad_array_new_length`` when the requested size
   exceeds the maximum supported size, as required by the C++ standard.
   Previously the type ``std::length_error`` was used.
 
+ABI Changes
+-----------
+
+- The C++17 variable templates ``is_error_code_enum_v`` and
+  ``is_error_condition_enum_v`` are now of type ``bool`` instead of ``size_t``.
+
 Build System Changes
 --------------------
 
 - Building the libc++ shared or static library requires a C++ 20 capable compiler.
   Consider using a Bootstrapping build to build libc++ with a fresh Clang if you
   can't use the system compiler to build libc++ anymore.
 
 - Historically, there has been numerous ways of building libc++ and libc++abi. This has

libcxx/include/system_error

 
 template <class T> struct is_error_code_enum
     : public false_type {};
 
 template <class T> struct is_error_condition_enum
     : public false_type {};
 
 template <class _Tp>
-inline constexpr size_t is_error_condition_enum_v = is_error_condition_enum<_Tp>::value; // C++17
+inline constexpr bool is_error_condition_enum_v = is_error_condition_enum<_Tp>::value; // C++17
 
 template <class _Tp>
-inline constexpr size_t is_error_code_enum_v = is_error_code_enum<_Tp>::value; // C++17
+inline constexpr bool is_error_code_enum_v = is_error_code_enum<_Tp>::value; // C++17
 
 class error_code
 {
 public:
     // constructors:
     error_code() noexcept;
     error_code(int val, const error_category& cat) noexcept;
     template <class ErrorCodeEnum>

 // is_error_code_enum
 
 template <class _Tp>
 struct _LIBCPP_TEMPLATE_VIS is_error_code_enum
     : public false_type {};
 
 #if _LIBCPP_STD_VER > 14
 template <class _Tp>
-inline constexpr size_t is_error_code_enum_v = is_error_code_enum<_Tp>::value;
+inline constexpr bool is_error_code_enum_v = is_error_code_enum<_Tp>::value;
 #endif
 
 // is_error_condition_enum
 
 template <class _Tp>
 struct _LIBCPP_TEMPLATE_VIS is_error_condition_enum
     : public false_type {};
 
 #if _LIBCPP_STD_VER > 14
 template <class _Tp>
-inline constexpr size_t is_error_condition_enum_v = is_error_condition_enum<_Tp>::value;
+inline constexpr bool is_error_condition_enum_v = is_error_condition_enum<_Tp>::value;
 #endif
 
 template <>
 struct _LIBCPP_TEMPLATE_VIS is_error_condition_enum<errc>
     : true_type { };
 
 #ifdef _LIBCPP_HAS_NO_STRONG_ENUMS
 template <>

libcxx/test/std/diagnostics/syserr/is_error_code_enum.pass.cpp

 
 template <bool Expected, class T>
 void
 test()
 {
     static_assert((std::is_error_code_enum<T>::value == Expected), "");
 #if TEST_STD_VER > 14
     static_assert((std::is_error_code_enum_v<T>      == Expected), "");
+    ASSERT_SAME_TYPE(decltype(std::is_error_code_enum_v<T>), const bool);
 #endif
 }
 
 class A {
     A();
     operator std::error_code () const { return std::error_code(); }
 };
 

libcxx/test/std/diagnostics/syserr/is_error_condition_enum.pass.cpp

 
 template <bool Expected, class T>
 void
 test()
 {
     static_assert((std::is_error_condition_enum<T>::value == Expected), "");
 #if TEST_STD_VER > 14
     static_assert((std::is_error_condition_enum_v<T>        == Expected), "");
+    ASSERT_SAME_TYPE(decltype(std::is_error_condition_enum_v<T>), const bool);
 #endif
 }
 
 class A {
     A();
     operator std::error_condition () const { return std::error_condition(); }
 };