Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[libc++][RFC] Always define internal feature test macros #89178

Merged
merged 1 commit into from
Oct 12, 2024

Conversation

philnik777
Copy link
Contributor

@philnik777 philnik777 commented Apr 18, 2024

Currently, the library-internal feature test macros are only defined if the feature is not available, and always have the prefix _LIBCPP_HAS_NO_. This patch changes that, so that they are always defined and have the prefix _LIBCPP_HAS_ instead. This changes the canonical use of these macros to #if _LIBCPP_HAS_FEATURE, which means that using an undefined macro (e.g. due to a missing include) is diagnosed now. While this is rather unlikely currently, a similar change in <__configuration/availability.h> caught a few bugs. This also improves readability, since it removes the double-negation of #ifndef _LIBCPP_HAS_NO_FEATURE.

The current patch only touches the macros defined in <__config>. If people are happy with this approach, I'll make a follow-up PR to also change the macros defined in <__config_site>.

@philnik777 philnik777 requested a review from a team as a code owner April 18, 2024 07:29
@llvmbot llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Apr 18, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented Apr 18, 2024

@llvm/pr-subscribers-libcxxabi

@llvm/pr-subscribers-libcxx

Author: Nikolas Klauser (philnik777)

Changes

Currently, the library-internal feature test macros are only defined if the feature is not available, and always have the prefix _LIBCPP_HAS_NO_. This patch changes that, so that they are always defined and have the prefix _LIBCPP_HAS_ instead. This changes the canonical use of these macros to #if _LIBCPP_HAS_FEATURE, which means that missing includes are diagnosed now. While this is rather unlikely currently, a similar change in &lt;__availability&gt; caught a few bugs. This also improves readability, since it removes the double-negation of #ifndef _LIBCPP_HAS_NO_FEATURE.

The current patch only touches the macros defined in &lt;__config&gt;. If people are happy with this approach, I'll make a follow-up PR to also change the macros defined in &lt;__config_site&gt;.


Patch is 205.98 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/89178.diff

111 Files Affected:

  • (modified) libcxx/include/__atomic/aliases.h (+1-1)
  • (modified) libcxx/include/__atomic/atomic_lock_free.h (+2-2)
  • (modified) libcxx/include/__availability (+4-4)
  • (modified) libcxx/include/__bit/byteswap.h (+2-2)
  • (modified) libcxx/include/__bit/countl.h (+2-2)
  • (modified) libcxx/include/__charconv/tables.h (+1-1)
  • (modified) libcxx/include/__charconv/to_chars_base_10.h (+1-1)
  • (modified) libcxx/include/__charconv/to_chars_integral.h (+1-1)
  • (modified) libcxx/include/__charconv/traits.h (+1-1)
  • (modified) libcxx/include/__chrono/file_clock.h (+1-1)
  • (modified) libcxx/include/__config (+47-21)
  • (modified) libcxx/include/__debug_utils/sanitizers.h (+5-5)
  • (modified) libcxx/include/__exception/exception_ptr.h (+1-1)
  • (modified) libcxx/include/__exception/nested_exception.h (+2-2)
  • (modified) libcxx/include/__expected/expected.h (+1-1)
  • (modified) libcxx/include/__filesystem/filesystem_error.h (+1-1)
  • (modified) libcxx/include/__filesystem/path.h (+6-6)
  • (modified) libcxx/include/__filesystem/u8path.h (+3-3)
  • (modified) libcxx/include/__format/format_arg.h (+9-9)
  • (modified) libcxx/include/__format/format_arg_store.h (+2-2)
  • (modified) libcxx/include/__format/format_error.h (+1-1)
  • (modified) libcxx/include/__format/format_functions.h (+2-2)
  • (modified) libcxx/include/__format/formatter_integer.h (+2-2)
  • (modified) libcxx/include/__functional/function.h (+20-20)
  • (modified) libcxx/include/__functional/hash.h (+3-3)
  • (modified) libcxx/include/__fwd/string.h (+5-5)
  • (modified) libcxx/include/__fwd/string_view.h (+2-2)
  • (modified) libcxx/include/__hash_table (+12-12)
  • (modified) libcxx/include/__iterator/counted_iterator.h (+2-2)
  • (modified) libcxx/include/__locale (+3-3)
  • (modified) libcxx/include/__memory/aligned_alloc.h (+3-3)
  • (modified) libcxx/include/__memory/shared_ptr.h (+25-25)
  • (modified) libcxx/include/__memory/temporary_buffer.h (+1-1)
  • (modified) libcxx/include/__memory/uninitialized_algorithms.h (+21-21)
  • (modified) libcxx/include/__pstl/backends/libdispatch.h (+2-2)
  • (modified) libcxx/include/__random/is_valid.h (+2-2)
  • (modified) libcxx/include/__random/log2.h (+4-4)
  • (modified) libcxx/include/__split_buffer (+4-4)
  • (modified) libcxx/include/__std_clang_module (+2-2)
  • (modified) libcxx/include/__string/char_traits.h (+2-2)
  • (modified) libcxx/include/__system_error/system_error.h (+1-1)
  • (modified) libcxx/include/__type_traits/is_integral.h (+2-2)
  • (modified) libcxx/include/__type_traits/is_signed_integer.h (+1-1)
  • (modified) libcxx/include/__type_traits/is_swappable.h (+1-1)
  • (modified) libcxx/include/__type_traits/is_unsigned_integer.h (+1-1)
  • (modified) libcxx/include/__type_traits/make_32_64_or_128_bit.h (+1-1)
  • (modified) libcxx/include/__type_traits/make_signed.h (+3-3)
  • (modified) libcxx/include/__type_traits/make_unsigned.h (+3-3)
  • (modified) libcxx/include/__type_traits/promote.h (+1-1)
  • (modified) libcxx/include/__utility/convert_to_integral.h (+1-1)
  • (modified) libcxx/include/__utility/exception_guard.h (+2-2)
  • (modified) libcxx/include/any (+6-6)
  • (modified) libcxx/include/atomic (+1-1)
  • (modified) libcxx/include/cuchar (+1-1)
  • (modified) libcxx/include/deque (+21-21)
  • (modified) libcxx/include/experimental/__simd/utility.h (+1-1)
  • (modified) libcxx/include/forward_list (+8-8)
  • (modified) libcxx/include/fstream (+4-4)
  • (modified) libcxx/include/future (+35-35)
  • (modified) libcxx/include/iomanip (+16-16)
  • (modified) libcxx/include/ios (+2-2)
  • (modified) libcxx/include/iosfwd (+1-1)
  • (modified) libcxx/include/istream (+77-77)
  • (modified) libcxx/include/list (+16-16)
  • (modified) libcxx/include/new (+6-6)
  • (modified) libcxx/include/optional (+1-1)
  • (modified) libcxx/include/ostream (+97-97)
  • (modified) libcxx/include/regex (+1-1)
  • (modified) libcxx/include/sstream (+5-5)
  • (modified) libcxx/include/stdexcept (+8-8)
  • (modified) libcxx/include/string (+21-21)
  • (modified) libcxx/include/string_view (+2-2)
  • (modified) libcxx/include/syncstream (+8-8)
  • (modified) libcxx/include/typeinfo (+1-1)
  • (modified) libcxx/include/valarray (+36-36)
  • (modified) libcxx/include/variant (+2-2)
  • (modified) libcxx/include/vector (+26-26)
  • (modified) libcxx/include/version (+1-1)
  • (modified) libcxx/modules/std.compat/cuchar.inc (+1-1)
  • (modified) libcxx/modules/std.cppm.in (+1-1)
  • (modified) libcxx/modules/std/atomic.inc (+1-1)
  • (modified) libcxx/modules/std/cuchar.inc (+1-1)
  • (modified) libcxx/modules/std/iosfwd.inc (+1-1)
  • (modified) libcxx/modules/std/memory.inc (+2-2)
  • (modified) libcxx/modules/std/string.inc (+2-2)
  • (modified) libcxx/modules/std/string_view.inc (+1-1)
  • (modified) libcxx/src/filesystem/error.h (+8-8)
  • (modified) libcxx/src/filesystem/format_string.h (+4-4)
  • (modified) libcxx/src/filesystem/int128_builtins.cpp (+1-1)
  • (modified) libcxx/src/future.cpp (+2-2)
  • (modified) libcxx/src/ios.cpp (+4-4)
  • (modified) libcxx/src/locale.cpp (+19-19)
  • (modified) libcxx/src/memory_resource.cpp (+4-4)
  • (modified) libcxx/src/new.cpp (+6-6)
  • (modified) libcxx/src/new_helpers.cpp (+1-1)
  • (modified) libcxx/src/ostream.cpp (+2-2)
  • (modified) libcxx/src/stdexcept.cpp (+1-1)
  • (modified) libcxx/src/support/runtime/exception_fallback.ipp (+4-4)
  • (modified) libcxx/src/support/runtime/exception_msvc.ipp (+4-4)
  • (modified) libcxx/src/system_error.cpp (+1-1)
  • (modified) libcxx/test/libcxx/concepts/concepts.arithmetic/__libcpp_integer.compile.pass.cpp (+2-2)
  • (modified) libcxx/test/libcxx/concepts/concepts.arithmetic/__libcpp_signed_integer.compile.pass.cpp (+2-2)
  • (modified) libcxx/test/libcxx/concepts/concepts.arithmetic/__libcpp_unsigned_integer.compile.pass.cpp (+2-2)
  • (modified) libcxx/test/libcxx/memory/aligned_allocation_macro.compile.pass.cpp (+1-1)
  • (modified) libcxx/test/libcxx/utilities/expected/expected.expected/value.observers.verify.cpp (+1-1)
  • (modified) libcxx/test/std/atomics/stdatomic.h.syn/types.compile.pass.cpp (+1-1)
  • (modified) libcxx/test/std/strings/c.strings/no_c8rtomb_mbrtoc8.verify.cpp (+1-1)
  • (modified) libcxx/test/support/test.support/make_string_header.pass.cpp (+1-1)
  • (modified) libcxx/test/support/test_macros.h (+5-6)
  • (modified) libcxx/utils/generate_feature_test_macro_components.py (+1-1)
  • (modified) libcxx/utils/libcxx/header_information.py (+2-2)
diff --git a/libcxx/include/__atomic/aliases.h b/libcxx/include/__atomic/aliases.h
index e27e09af6b77d9..afc64eaaa69e7b 100644
--- a/libcxx/include/__atomic/aliases.h
+++ b/libcxx/include/__atomic/aliases.h
@@ -37,7 +37,7 @@ using atomic_long   = atomic<long>;
 using atomic_ulong  = atomic<unsigned long>;
 using atomic_llong  = atomic<long long>;
 using atomic_ullong = atomic<unsigned long long>;
-#ifndef _LIBCPP_HAS_NO_CHAR8_T
+#if _LIBCPP_HAS_CHAR8_T
 using atomic_char8_t = atomic<char8_t>;
 #endif
 using atomic_char16_t = atomic<char16_t>;
diff --git a/libcxx/include/__atomic/atomic_lock_free.h b/libcxx/include/__atomic/atomic_lock_free.h
index 0715439db45039..3ae9b8856e8102 100644
--- a/libcxx/include/__atomic/atomic_lock_free.h
+++ b/libcxx/include/__atomic/atomic_lock_free.h
@@ -18,7 +18,7 @@
 #if defined(__CLANG_ATOMIC_BOOL_LOCK_FREE)
 #  define ATOMIC_BOOL_LOCK_FREE __CLANG_ATOMIC_BOOL_LOCK_FREE
 #  define ATOMIC_CHAR_LOCK_FREE __CLANG_ATOMIC_CHAR_LOCK_FREE
-#  ifndef _LIBCPP_HAS_NO_CHAR8_T
+#  if _LIBCPP_HAS_CHAR8_T
 #    define ATOMIC_CHAR8_T_LOCK_FREE __CLANG_ATOMIC_CHAR8_T_LOCK_FREE
 #  endif
 #  define ATOMIC_CHAR16_T_LOCK_FREE __CLANG_ATOMIC_CHAR16_T_LOCK_FREE
@@ -32,7 +32,7 @@
 #elif defined(__GCC_ATOMIC_BOOL_LOCK_FREE)
 #  define ATOMIC_BOOL_LOCK_FREE __GCC_ATOMIC_BOOL_LOCK_FREE
 #  define ATOMIC_CHAR_LOCK_FREE __GCC_ATOMIC_CHAR_LOCK_FREE
-#  ifndef _LIBCPP_HAS_NO_CHAR8_T
+#  if _LIBCPP_HAS_CHAR8_T
 #    define ATOMIC_CHAR8_T_LOCK_FREE __GCC_ATOMIC_CHAR8_T_LOCK_FREE
 #  endif
 #  define ATOMIC_CHAR16_T_LOCK_FREE __GCC_ATOMIC_CHAR16_T_LOCK_FREE
diff --git a/libcxx/include/__availability b/libcxx/include/__availability
index aa761eb5bfe5e3..413c54d7a334cc 100644
--- a/libcxx/include/__availability
+++ b/libcxx/include/__availability
@@ -315,10 +315,10 @@
 
 #endif
 
-// Define availability attributes that depend on _LIBCPP_HAS_NO_EXCEPTIONS.
+// Define availability attributes that depend on _LIBCPP_HAS_EXCEPTIONS.
 // Those are defined in terms of the availability attributes above, and
 // should not be vendor-specific.
-#if defined(_LIBCPP_HAS_NO_EXCEPTIONS)
+#if !_LIBCPP_HAS_EXCEPTIONS
 #  define _LIBCPP_AVAILABILITY_THROW_BAD_ANY_CAST
 #  define _LIBCPP_AVAILABILITY_THROW_BAD_OPTIONAL_ACCESS
 #  define _LIBCPP_AVAILABILITY_THROW_BAD_VARIANT_ACCESS
@@ -329,8 +329,8 @@
 #endif
 
 // Define availability attributes that depend on both
-// _LIBCPP_HAS_NO_EXCEPTIONS and _LIBCPP_HAS_NO_RTTI.
-#if defined(_LIBCPP_HAS_NO_EXCEPTIONS) || defined(_LIBCPP_HAS_NO_RTTI)
+// _LIBCPP_HAS_EXCEPTIONS and _LIBCPP_HAS_RTTI.
+#if !_LIBCPP_HAS_EXCEPTIONS || !_LIBCPP_HAS_RTTI
 #  undef _LIBCPP_AVAILABILITY_HAS_INIT_PRIMARY_EXCEPTION
 #  undef _LIBCPP_AVAILABILITY_INIT_PRIMARY_EXCEPTION
 #  define _LIBCPP_AVAILABILITY_HAS_INIT_PRIMARY_EXCEPTION 0
diff --git a/libcxx/include/__bit/byteswap.h b/libcxx/include/__bit/byteswap.h
index 20045d6fd43cb5..6226862cdc9c63 100644
--- a/libcxx/include/__bit/byteswap.h
+++ b/libcxx/include/__bit/byteswap.h
@@ -32,7 +32,7 @@ _LIBCPP_NODISCARD_EXT _LIBCPP_HIDE_FROM_ABI constexpr _Tp byteswap(_Tp __val) no
     return __builtin_bswap32(__val);
   } else if constexpr (sizeof(_Tp) == 8) {
     return __builtin_bswap64(__val);
-#  ifndef _LIBCPP_HAS_NO_INT128
+#  if _LIBCPP_HAS_INT128
   } else if constexpr (sizeof(_Tp) == 16) {
 #    if __has_builtin(__builtin_bswap128)
     return __builtin_bswap128(__val);
@@ -40,7 +40,7 @@ _LIBCPP_NODISCARD_EXT _LIBCPP_HIDE_FROM_ABI constexpr _Tp byteswap(_Tp __val) no
     return static_cast<_Tp>(byteswap(static_cast<uint64_t>(__val))) << 64 |
            static_cast<_Tp>(byteswap(static_cast<uint64_t>(__val >> 64)));
 #    endif // __has_builtin(__builtin_bswap128)
-#  endif   // _LIBCPP_HAS_NO_INT128
+#  endif   // _LIBCPP_HAS_INT128
   } else {
     static_assert(sizeof(_Tp) == 0, "byteswap is unimplemented for integral types of this size");
   }
diff --git a/libcxx/include/__bit/countl.h b/libcxx/include/__bit/countl.h
index 13df8d4e66c402..767052ddabe913 100644
--- a/libcxx/include/__bit/countl.h
+++ b/libcxx/include/__bit/countl.h
@@ -39,7 +39,7 @@ _LIBCPP_NODISCARD inline _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR int __libcpp_cl
   return __builtin_clzll(__x);
 }
 
-#ifndef _LIBCPP_HAS_NO_INT128
+#if _LIBCPP_HAS_INT128
 inline _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR int __libcpp_clz(__uint128_t __x) _NOEXCEPT {
 #  if __has_builtin(__builtin_clzg)
   return __builtin_clzg(__x);
@@ -57,7 +57,7 @@ inline _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR int __libcpp_clz(__uint128_t __x)
                             : __builtin_clzll(static_cast<unsigned long long>(__x >> 64));
 #  endif
 }
-#endif // _LIBCPP_HAS_NO_INT128
+#endif // _LIBCPP_HAS_INT128
 
 template <class _Tp>
 _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX14 int __countl_zero(_Tp __t) _NOEXCEPT {
diff --git a/libcxx/include/__charconv/tables.h b/libcxx/include/__charconv/tables.h
index 6b93536b8c1bac..9568bf841cd029 100644
--- a/libcxx/include/__charconv/tables.h
+++ b/libcxx/include/__charconv/tables.h
@@ -95,7 +95,7 @@ inline constexpr uint64_t __pow10_64[20] = {
     UINT64_C(1000000000000000000),
     UINT64_C(10000000000000000000)};
 
-#  ifndef _LIBCPP_HAS_NO_INT128
+#  if _LIBCPP_HAS_INT128
 inline constexpr int __pow10_128_offset      = 0;
 inline constexpr __uint128_t __pow10_128[40] = {
     UINT64_C(0),
diff --git a/libcxx/include/__charconv/to_chars_base_10.h b/libcxx/include/__charconv/to_chars_base_10.h
index c49f4f6797aa43..06e4e692337df5 100644
--- a/libcxx/include/__charconv/to_chars_base_10.h
+++ b/libcxx/include/__charconv/to_chars_base_10.h
@@ -124,7 +124,7 @@ __base_10_u64(char* __buffer, uint64_t __value) noexcept {
   return __itoa::__append10(__buffer, __value);
 }
 
-#  ifndef _LIBCPP_HAS_NO_INT128
+#  if _LIBCPP_HAS_INT128
 /// \returns 10^\a exp
 ///
 /// \pre \a exp [19, 39]
diff --git a/libcxx/include/__charconv/to_chars_integral.h b/libcxx/include/__charconv/to_chars_integral.h
index 0369f4dfb9bda6..e7583bcdaa64b5 100644
--- a/libcxx/include/__charconv/to_chars_integral.h
+++ b/libcxx/include/__charconv/to_chars_integral.h
@@ -70,7 +70,7 @@ __to_chars_itoa(char* __first, char* __last, _Tp __value, false_type) {
     return {__last, errc::value_too_large};
 }
 
-#  ifndef _LIBCPP_HAS_NO_INT128
+#  if _LIBCPP_HAS_INT128
 template <>
 inline _LIBCPP_CONSTEXPR_SINCE_CXX23 _LIBCPP_HIDE_FROM_ABI to_chars_result
 __to_chars_itoa(char* __first, char* __last, __uint128_t __value, false_type) {
diff --git a/libcxx/include/__charconv/traits.h b/libcxx/include/__charconv/traits.h
index c91c6da3247978..2cb37c8cfb0238 100644
--- a/libcxx/include/__charconv/traits.h
+++ b/libcxx/include/__charconv/traits.h
@@ -88,7 +88,7 @@ struct _LIBCPP_HIDDEN __traits_base<_Tp, __enable_if_t<sizeof(_Tp) == sizeof(uin
   }
 };
 
-#  ifndef _LIBCPP_HAS_NO_INT128
+#  if _LIBCPP_HAS_INT128
 template <typename _Tp>
 struct _LIBCPP_HIDDEN __traits_base<_Tp, __enable_if_t<sizeof(_Tp) == sizeof(__uint128_t)> > {
   using type = __uint128_t;
diff --git a/libcxx/include/__chrono/file_clock.h b/libcxx/include/__chrono/file_clock.h
index 7d25729fec013a..f791614c253909 100644
--- a/libcxx/include/__chrono/file_clock.h
+++ b/libcxx/include/__chrono/file_clock.h
@@ -48,7 +48,7 @@ _LIBCPP_END_NAMESPACE_STD
 #ifndef _LIBCPP_CXX03_LANG
 _LIBCPP_BEGIN_NAMESPACE_FILESYSTEM
 struct _FilesystemClock {
-#  if !defined(_LIBCPP_HAS_NO_INT128)
+#  if _LIBCPP_HAS_INT128
   typedef __int128_t rep;
   typedef nano period;
 #  else
diff --git a/libcxx/include/__config b/libcxx/include/__config
index 4ccef2ca0d73b4..f2e12e9106d937 100644
--- a/libcxx/include/__config
+++ b/libcxx/include/__config
@@ -533,6 +533,7 @@ _LIBCPP_HARDENING_MODE_DEBUG
 #    define _NOEXCEPT noexcept
 #    define _NOEXCEPT_(...) noexcept(__VA_ARGS__)
 #    define _LIBCPP_CONSTEXPR constexpr
+#    define _LIBCPP_HAS_NOEXCEPT 1
 
 #  else
 
@@ -540,7 +541,7 @@ _LIBCPP_HARDENING_MODE_DEBUG
 #    define _ALIGNAS_TYPE(x) __attribute__((__aligned__(_LIBCPP_ALIGNOF(x))))
 #    define _ALIGNAS(x) __attribute__((__aligned__(x)))
 #    define _LIBCPP_NORETURN __attribute__((__noreturn__))
-#    define _LIBCPP_HAS_NO_NOEXCEPT
+#    define _LIBCPP_HAS_NOEXCEPT 0
 #    define nullptr __nullptr
 #    define _NOEXCEPT throw()
 #    define _NOEXCEPT_(...)
@@ -554,7 +555,9 @@ typedef __char32_t char32_t;
 #  endif
 
 #  if !defined(__cpp_exceptions) || __cpp_exceptions < 199711L
-#    define _LIBCPP_HAS_NO_EXCEPTIONS
+#    define _LIBCPP_HAS_EXCEPTIONS 0
+#  else
+#    define _LIBCPP_HAS_EXCEPTIONS 1
 #  endif
 
 #  define _LIBCPP_PREFERRED_ALIGNOF(_Tp) __alignof(_Tp)
@@ -591,8 +594,10 @@ typedef __char32_t char32_t;
 #      define _LIBCPP_HAS_BLOCKS_RUNTIME
 #    endif
 
-#    if !__has_feature(address_sanitizer)
-#      define _LIBCPP_HAS_NO_ASAN
+#    if __has_feature(address_sanitizer)
+#      define _LIBCPP_HAS_ASAN 1
+#    else
+#      define _LIBCPP_HAS_ASAN 0
 #    endif
 
 #    define _LIBCPP_ALWAYS_INLINE __attribute__((__always_inline__))
@@ -601,8 +606,10 @@ typedef __char32_t char32_t;
 
 #  elif defined(_LIBCPP_COMPILER_GCC)
 
-#    if !defined(__SANITIZE_ADDRESS__)
-#      define _LIBCPP_HAS_NO_ASAN
+#    if defined(__SANITIZE_ADDRESS__)
+#      define _LIBCPP_HAS_ASAN 1
+#    else
+#      define _LIBCPP_HAS_ASAN 0
 #    endif
 
 #    define _LIBCPP_ALWAYS_INLINE __attribute__((__always_inline__))
@@ -727,7 +734,7 @@ typedef __char32_t char32_t;
 #    define _LIBCPP_HARDENING_SIG n // "none"
 #  endif
 
-#  ifdef _LIBCPP_HAS_NO_EXCEPTIONS
+#  if !_LIBCPP_HAS_EXCEPTIONS
 #    define _LIBCPP_EXCEPTIONS_SIG n
 #  else
 #    define _LIBCPP_EXCEPTIONS_SIG e
@@ -856,7 +863,9 @@ typedef __char32_t char32_t;
 #  endif
 
 #  if !defined(__SIZEOF_INT128__) || defined(_MSC_VER)
-#    define _LIBCPP_HAS_NO_INT128
+#    define _LIBCPP_HAS_INT128 0
+#  else
+#    define _LIBCPP_HAS_INT128 1
 #  endif
 
 #  ifdef _LIBCPP_CXX03_LANG
@@ -888,17 +897,21 @@ typedef __char32_t char32_t;
 // If we are getting operator new from the MSVC CRT, then allocation overloads
 // for align_val_t were added in 19.12, aka VS 2017 version 15.3.
 #  if defined(_LIBCPP_MSVCRT) && defined(_MSC_VER) && _MSC_VER < 1912
-#    define _LIBCPP_HAS_NO_LIBRARY_ALIGNED_ALLOCATION
+#    define _LIBCPP_HAS_LIBRARY_ALIGNED_ALLOCATION 0
 #  elif defined(_LIBCPP_ABI_VCRUNTIME) && !defined(__cpp_aligned_new)
 // We're deferring to Microsoft's STL to provide aligned new et al. We don't
 // have it unless the language feature test macro is defined.
-#    define _LIBCPP_HAS_NO_LIBRARY_ALIGNED_ALLOCATION
+#    define _LIBCPP_HAS_LIBRARY_ALIGNED_ALLOCATION 0
 #  elif defined(__MVS__)
-#    define _LIBCPP_HAS_NO_LIBRARY_ALIGNED_ALLOCATION
+#    define _LIBCPP_HAS_LIBRARY_ALIGNED_ALLOCATION 0
+#  else
+#    define _LIBCPP_HAS_LIBRARY_ALIGNED_ALLOCATION 1
 #  endif
 
-#  if defined(_LIBCPP_HAS_NO_LIBRARY_ALIGNED_ALLOCATION) || (!defined(__cpp_aligned_new) || __cpp_aligned_new < 201606)
-#    define _LIBCPP_HAS_NO_ALIGNED_ALLOCATION
+#  if !_LIBCPP_HAS_LIBRARY_ALIGNED_ALLOCATION || (!defined(__cpp_aligned_new) || __cpp_aligned_new < 201606)
+#    define _LIBCPP_HAS_ALIGNED_ALLOCATION 0
+#  else
+#    define _LIBCPP_HAS_ALIGNED_ALLOCATION 1
 #  endif
 
 // It is not yet possible to use aligned_alloc() on all Apple platforms since
@@ -906,11 +919,15 @@ typedef __char32_t char32_t;
 #  if defined(__APPLE__)
 #    if (defined(__ENVIRONMENT_MAC_OS_X_VERSION_MIN_REQUIRED__) &&                                                     \
          __ENVIRONMENT_MAC_OS_X_VERSION_MIN_REQUIRED__ < 101500)
-#      define _LIBCPP_HAS_NO_C11_ALIGNED_ALLOC
+#      define _LIBCPP_HAS_C11_ALIGNED_ALLOC 0
+#    else
+#      define _LIBCPP_HAS_C11_ALIGNED_ALLOC 1
 #    endif
 #  elif defined(__ANDROID__) && __ANDROID_API__ < 28
 // Android only provides aligned_alloc when targeting API 28 or higher.
-#    define _LIBCPP_HAS_NO_C11_ALIGNED_ALLOC
+#    define _LIBCPP_HAS_C11_ALIGNED_ALLOC 0
+#  else
+#    define _LIBCPP_HAS_C11_ALIGNED_ALLOC 1
 #  endif
 
 #  if defined(__APPLE__) || defined(__FreeBSD__)
@@ -922,7 +939,9 @@ typedef __char32_t char32_t;
 #  endif
 
 #  if _LIBCPP_STD_VER <= 17 || !defined(__cpp_char8_t)
-#    define _LIBCPP_HAS_NO_CHAR8_T
+#    define _LIBCPP_HAS_CHAR8_T 0
+#  else
+#    define _LIBCPP_HAS_CHAR8_T 1
 #  endif
 
 // Deprecation macros.
@@ -989,7 +1008,7 @@ typedef __char32_t char32_t;
 #    define _LIBCPP_DEPRECATED_IN_CXX26
 #  endif
 
-#  if !defined(_LIBCPP_HAS_NO_CHAR8_T)
+#  if _LIBCPP_HAS_CHAR8_T
 #    define _LIBCPP_DEPRECATED_WITH_CHAR8_T _LIBCPP_DEPRECATED
 #  else
 #    define _LIBCPP_DEPRECATED_WITH_CHAR8_T
@@ -1044,7 +1063,9 @@ typedef __char32_t char32_t;
 
 // Try to find out if RTTI is disabled.
 #  if !defined(__cpp_rtti) || __cpp_rtti < 199711L
-#    define _LIBCPP_HAS_NO_RTTI
+#    define _LIBCPP_HAS_RTTI 0
+#  else
+#    define _LIBCPP_HAS_RTTI 1
 #  endif
 
 #  ifndef _LIBCPP_WEAK
@@ -1153,8 +1174,9 @@ typedef __char32_t char32_t;
 
 #  if !defined(_LIBCPP_HAS_C_ATOMIC_IMP) && !defined(_LIBCPP_HAS_GCC_ATOMIC_IMP) &&                                    \
       !defined(_LIBCPP_HAS_EXTERNAL_ATOMIC_IMP)
-#    define _LIBCPP_HAS_NO_ATOMIC_HEADER
+#    define _LIBCPP_HAS_ATOMIC_HEADER 0
 #  else
+#    define _LIBCPP_HAS_ATOMIC_HEADER 1
 #    ifndef _LIBCPP_ATOMIC_FLAG_TYPE
 #      define _LIBCPP_ATOMIC_FLAG_TYPE bool
 #    endif
@@ -1271,7 +1293,7 @@ typedef __char32_t char32_t;
 // functions is gradually being added to existing C libraries. The conditions
 // below check for known C library versions and conditions under which these
 // functions are declared by the C library.
-#  define _LIBCPP_HAS_NO_C8RTOMB_MBRTOC8
+
 // GNU libc 2.36 and newer declare c8rtomb() and mbrtoc8() in C++ modes if
 // __cpp_char8_t is defined or if C2X extensions are enabled. Determining
 // the latter depends on internal GNU libc details that are not appropriate
@@ -1279,8 +1301,12 @@ typedef __char32_t char32_t;
 // defined are ignored.
 #  if defined(_LIBCPP_GLIBC_PREREQ)
 #    if _LIBCPP_GLIBC_PREREQ(2, 36) && defined(__cpp_char8_t)
-#      undef _LIBCPP_HAS_NO_C8RTOMB_MBRTOC8
+#      define _LIBCPP_HAS_C8RTOMB_MBRTOC8 1
+#    else
+#      define _LIBCPP_HAS_C8RTOMB_MBRTOC8 0
 #    endif
+#  else
+#    define _LIBCPP_HAS_C8RTOMB_MBRTOC8 0
 #  endif
 
 // There are a handful of public standard library types that are intended to
diff --git a/libcxx/include/__debug_utils/sanitizers.h b/libcxx/include/__debug_utils/sanitizers.h
index d8547e32493303..73d192711eabb6 100644
--- a/libcxx/include/__debug_utils/sanitizers.h
+++ b/libcxx/include/__debug_utils/sanitizers.h
@@ -17,7 +17,7 @@
 #  pragma GCC system_header
 #endif
 
-#ifndef _LIBCPP_HAS_NO_ASAN
+#if _LIBCPP_HAS_ASAN
 
 extern "C" {
 _LIBCPP_EXPORTED_FROM_ABI void
@@ -28,12 +28,12 @@ _LIBCPP_EXPORTED_FROM_ABI int
 __sanitizer_verify_double_ended_contiguous_container(const void*, const void*, const void*, const void*);
 }
 
-#endif // _LIBCPP_HAS_NO_ASAN
+#endif // _LIBCPP_HAS_ASAN
 
 _LIBCPP_BEGIN_NAMESPACE_STD
 
 // ASan choices
-#ifndef _LIBCPP_HAS_NO_ASAN
+#if _LIBCPP_HAS_ASAN
 #  define _LIBCPP_HAS_ASAN_CONTAINER_ANNOTATIONS_FOR_ALL_ALLOCATORS 1
 #endif
 
@@ -57,7 +57,7 @@ _LIBCPP_HIDE_FROM_ABI void __annotate_double_ended_contiguous_container(
     const void* __last_old_contained,
     const void* __first_new_contained,
     const void* __last_new_contained) {
-#ifdef _LIBCPP_HAS_NO_ASAN
+#if !_LIBCPP_HAS_ASAN
   (void)__first_storage;
   (void)__last_storage;
   (void)__first_old_contained;
@@ -86,7 +86,7 @@ _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX14 void __annotate_contiguous_c
     const void* __last_storage,
     const void* __old_last_contained,
     const void* __new_last_contained) {
-#ifdef _LIBCPP_HAS_NO_ASAN
+#if !_LIBCPP_HAS_ASAN
   (void)__first_storage;
   (void)__last_storage;
   (void)__old_last_contained;
diff --git a/libcxx/include/__exception/exception_ptr.h b/libcxx/include/__exception/exception_ptr.h
index c9027de9238cdd..b0c59017d9424a 100644
--- a/libcxx/include/__exception/exception_ptr.h
+++ b/libcxx/include/__exception/exception_ptr.h
@@ -87,7 +87,7 @@ class _LIBCPP_EXPORTED_FROM_ABI exception_ptr {
 
 template <class _Ep>
 _LIBCPP_HIDE_FROM_ABI exception_ptr make_exception_ptr(_Ep __e) _NOEXCEPT {
-#  ifndef _LIBCPP_HAS_NO_EXCEPTIONS
+#  if _LIBCPP_HAS_EXCEPTIONS
 #    if _LIBCPP_AVAILABILITY_HAS_INIT_PRIMARY_EXCEPTION && __cplusplus >= 201103L
   using _Ep2 = __decay_t<_Ep>;
 
diff --git a/libcxx/include/__exception/nested_exception.h b/libcxx/include/__exception/nested_exception.h
index 1bf2df939258a6..4fa49e7a9c7a7f 100644
--- a/libcxx/include/__exception/nested_exception.h
+++ b/libcxx/include/__exception/nested_exception.h
@@ -47,7 +47,7 @@ struct __nested : public _Tp, public nested_exception {
   _LIBCPP_HIDE_FROM_ABI explicit __nested(const _Tp& __t) : _Tp(__t) {}
 };
 
-#ifndef _LIBCPP_HAS_NO_EXCEPTIONS
+#if _LIBCPP_HAS_EXCEPTIONS
 template <class _Tp, class _Up, bool>
 struct __throw_with_nested;
 
@@ -66,7 +66,7 @@ struct __throw_with_nested<_Tp, _Up, false> {
 
 template <class _Tp>
 _LIBCPP_NORETURN _LIBCPP_HIDE_FROM_ABI void throw_with_nested(_Tp&& __t) {
-#ifndef _LIBCPP_HAS_NO_EXCEPTIONS
+#if _LIBCPP_HAS_EXCEPTIONS
   using _Up = __decay_t<_Tp>;
   static_assert(is_copy_constructible<_Up>::value, "type thrown must be CopyConstructible");
   __throw_with_nested<_Tp,
diff --git a/libcxx/include/__expected/expected.h b/libcxx/include/__expected/expected.h
index d7adaac7567b2f..a60b0a3b6f8856 100644
--- a/libcxx/include/__expected/expected.h
+++ b/libcxx/include/__expected/expected.h
@@ -70,7 +70,7 @@ struct __expected_construct_unexpected_from_invoke_tag {};
 
 template <class _Err, class _Arg>
 _LIBCPP_HIDE_FROM_ABI void __throw_bad_expected_access(_Arg&& __arg) {
-#  ifndef _LIBCPP_HAS_NO_EXCEPTIONS
+#  if _LIBCPP_HAS_EXCEPTIONS
   throw bad_expected_access<_Err>(std::forward<_Arg>(__arg));
 #  else
   (void)__arg;
diff --git a/libcxx/include/__filesystem/filesystem_error.h b/libcxx/include/__filesystem/filesystem_error.h
index bfdcc5eaee521f..822a428d02ba6b 100644
--- a/libcxx/include/__filesystem/filesystem_error.h
+++ b/libcxx/include/__filesystem/filesystem_error.h
@@ -68,7 +68,7 @@ class _LIBCPP_AVAILABILITY_FILESYSTEM_LIBRARY _LIBCPP_EXPORTED_FROM_ABI filesyst
   shared_ptr<_Storage> __storage_;
 };
 
-#  ifndef _LIBCPP_HAS_NO_EXCEPTIONS
+#  if _LIBCPP_HAS_EXCEPTIONS
 template <class... _Args>
 _LIBCPP_NORETURN inline _LIBCPP_HIDE_FROM_ABI _LIBCPP_AVAILABILITY_FILESYSTEM_LIBRARY void
 __throw_filesystem_error(_Args&&... __args) {
diff --git a/libcxx/include/__filesystem/path.h b/libcxx/include/__filesystem/path.h
index 9ffc90ada5e716..d8aa0af93ac9e0 100644
--- a/libcxx/include/__filesystem/path.h
+++ b/libcxx/include/__filesystem/path.h
@@ -60,7 +60,7 @@ struct __can_convert_char<wchar_t> {
   static const bool value = true;
   using __char_type       = wchar_t;
 };
-#  ifndef _LIBCPP_HAS_NO_CHAR8_T
+#  if _LIBCPP_HAS_CHAR8_T
 template <>
 struct __can_convert_char<char8_t> {
   static const bool value = true;
@@ -87,7 +87,7 @@ _LIBCPP_HIDE_FROM_ABI bool __is_separator(_ECharT __e) {
 #  endif
 }
 
-#  ifndef _LIBCPP_HAS_NO_CHAR8_T
+#  if _LIBCPP_HAS_CHAR8_T
 typedef u8string __u8_string;
 #  else
 typedef string __u8_string;
@@ -366,7 +366,7 @@ struct _PathExport<char16_t> {
   }
 };
 
-#    ifndef _LIBCPP_HAS_NO_CHAR8_T
+#    if _LIBCPP_HAS_CHAR8_T
 template <>
 struct _PathExport<char8_t> {
   typedef __narrow_to_utf8<sizeof(wchar_t) * __CHAR_BIT__> _Narrower;
@@ -376,7 +376,7 @@ struct _PathExport<char8_t> {
     _Narrower()(back_inserter(__dest), __src.data(), __src.data() + __src.size());
   }
 };
-#    endif /* !_LIBCPP_HAS_NO_CHAR8_T */
+#    endif // _LIBCPP_HAS_CHAR8_T
 #  endif   /* _LIBCPP_WIN32API */
 
 class _LIBCPP_EXPORTED_FROM_ABI path {
@@ -730,7 +730,7 @@ class _LIBCPP_EXPORTED_FROM_ABI path {
 #  else    /* _LIBCPP_WIN32API */
 
   _LIBCPP_HIDE_FROM_ABI std::string string() const { return __pn_; }
-#    ifndef _LIBCPP_HAS_NO_CHAR8_T
+#    if _LIBCPP_HAS_CHAR8_T
   _LIBCPP_HIDE_FROM_ABI std::u8string u8string() const { return std::u8string(__pn_.begin(), __pn_.end()); }
 #    else
   _LIBCPP_HIDE_FROM_ABI std::string u8string() const { return __pn_; }
@@ -756,7 +756,7 @@ class _LIBCPP_EXPORTED_FROM_ABI path {
 
   // generic format observers
   _LIBCPP_HIDE_FROM_ABI std::string generic_string() const { return __pn_; }
-#    ifndef _LIBCPP_HAS_NO_CHAR8_T
+#    if _LIBCPP_HAS_CHAR8_T
   _LIBCPP_HIDE_FROM_ABI std::u8string generic_u8string() const...
[truncated]

Copy link

github-actions bot commented Apr 18, 2024

✅ With the latest revision this PR passed the C/C++ code formatter.

Copy link
Member

@ldionne ldionne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I generally like this change.

On the one hand, I love getting rid of the double negative, the fact that we'll trigger -Wundef and the fact that we make it a bit clearer what is intended to be set by the library and not by users.

On the other hand, this change makes it look a bit like _LIBCPP_HAS_EXCEPTION is an "optional" feature just like having ASAN turned on. I guess they are actually the same and I just don't think about them that way, but it could be that _LIBCPP_HAS_NO_EXCEPTIONS has been there for so long and is so prevalent that it has a special place in my mind.

I am generally happy with this patch, but I expect it will require a bit of adjustment for downstreams. Pinging @llvm/libcxx-vendors for awareness.

I'm eager to know what other folks think about this, so I won't approve just yet. Let's let this bake for at least a few days.

@rupprecht
Copy link
Collaborator

What kinds of adjustments downstream do you expect this will require?

Maybe it's just something historical, but I see some -D_LIBCPP_HAS_NO_<x> usage. Does this PR mean we should use -D_LIBCPP_HAS_NO_FOO -D_LIBCPP_HAS_FOO=0 as a way to be compatible before & after this change?

OTOH, it seems suspicious that we're setting these in the first place. If the conditions in __config lead to libc++ defining #define _LIBCPP_HAS_FOO 1 but we have -D_LIBCPP_HAS_FOO=0 as a build flag, that would break due to macro redefinition w/ a different value, right? Are there any conditions here where it would make sense for __config to only define it if not already defined?

One common option I see being set is _LIBCPP_HAS_NO_THREADS, but that isn't being changed here. Did you miss that one, or is intentional that only a subset of the HAS_NO vars are being changed?

+1 to this patch being an improvement, especially with the removal of double-negative as mentioned.

@ldionne
Copy link
Member

ldionne commented Apr 18, 2024

What kinds of adjustments downstream do you expect this will require?

Since this touches a lot of widely used macros, any downstream-only code that is conditionalized on exceptions would likely use _LIBCPP_HAS_NO_EXCEPTIONS and would need to change. That's just an example, but you get the idea.

Maybe it's just something historical, but I see some -D_LIBCPP_HAS_NO_<x> usage. Does this PR mean we should use -D_LIBCPP_HAS_NO_FOO -D_LIBCPP_HAS_FOO=0 as a way to be compatible before & after this change?

If you can share which FOOs you found these usages for, that could also be enlightening.

OTOH, it seems suspicious that we're setting these in the first place. If the conditions in __config lead to libc++ defining #define _LIBCPP_HAS_FOO 1 but we have -D_LIBCPP_HAS_FOO=0 as a build flag, that would break due to macro redefinition w/ a different value, right? Are there any conditions here where it would make sense for __config to only define it if not already defined?

Exactly, right. So libc++ has a notion of "user configurable settings", but not all the _LIBCPP_HAS_NO_FOO fall into that category. Unfortunately, whether something is intended to be user-configurable or not is usually not well documented and it's mostly something that the developers know.

In the general case, users are not expected to define any _LIBCPP macro unless it's documented as a public configurable thing. In practice, people probably often define macros in ways we haven't thought about.

One common option I see being set is _LIBCPP_HAS_NO_THREADS, but that isn't being changed here. Did you miss that one, or is intentional that only a subset of the HAS_NO vars are being changed?

It looks like an oversight on @philnik777 's part. It also seems incorrect for users to be setting that, since this should be done via __config_site at configuration-time of the library.

IMO this patch can be a first step towards formalizing what is and isn't a user-configurable setting. We could potentially use a different naming convention to make it more obvious what is configurable. Or we could potentially do something like

#ifdef _LIBCPP_HAS_EXCEPTIONS
#  error "_LIBCPP_HAS_EXCEPTIONS is not supposed to be user-configured, it's supposed to be inferred by libc++"
#endif

#if __has_feature(__cxx_exceptions)
#  define _LIBCPP_HAS_EXCEPTIONS 1
#else
#  define _LIBCPP_HAS_EXCEPTIONS 0
#endif

That would make it clear what we intend for users to set and what we don't. But any such thing should be left for a different patch.

@ldionne
Copy link
Member

ldionne commented Apr 18, 2024

Or we could also use final macros, which I just learned about: https://clang.llvm.org/docs/LanguageExtensions.html#final-macros

#pragma clang final(_LIBCPP_HAS_EXCEPTIONS)
#if __has_feature(__cxx_exceptions)
#  define _LIBCPP_HAS_EXCEPTIONS 1
#else
#  define _LIBCPP_HAS_EXCEPTIONS 0
#endif

IDK if that behaves exactly how we want, but basically we have a few options on the table.

@rupprecht
Copy link
Collaborator

What kinds of adjustments downstream do you expect this will require?

Since this touches a lot of widely used macros, any downstream-only code that is conditionalized on exceptions would likely use _LIBCPP_HAS_NO_EXCEPTIONS and would need to change. That's just an example, but you get the idea.

Oh, I was only looking at build configurations, I completely forgot about that. I see quite a bit more when I expand the scope for things like that, e.g. https://github.com/abseil/abseil-cpp/blob/192e959b16809f751d565b53a949b21129d904fb/absl/hash/internal/hash.h#L73

Probably still tractable, but not as trivial as I thought.

It may help to temporarily leave in the old definitions for downstream use, even if libc++ itself does not use it, i.e.

#if __has_feature(__cxx_exceptions)
#  define _LIBCPP_HAS_EXCEPTIONS 1
#else
#  define _LIBCPP_HAS_EXCEPTIONS 0
#  ifdef _LIBCPP_DEFINE_DEPRECATED_FEATURE_TEST_MACROS  // <-- remove after an LLVM release or whatever
#    define _LIBCPP_HAS_NO_EXCEPTIONS
#  endif
#endif

(we may just need this for some popular subset of macros)

Maybe it's just something historical, but I see some -D_LIBCPP_HAS_NO_<x> usage. Does this PR mean we should use -D_LIBCPP_HAS_NO_FOO -D_LIBCPP_HAS_FOO=0 as a way to be compatible before & after this change?

If you can share which FOOs you found these usages for, that could also be enlightening.

_LIBCPP_HAS_NO_FILESYSTEM_LIBRARY
_LIBCPP_HAS_NO_INT128
_LIBCPP_HAS_NO_LIBRARY_ALIGNED_ALLOCATION
_LIBCPP_HAS_NO_LOCALIZATION
_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER
_LIBCPP_HAS_NO_STDIN
_LIBCPP_HAS_NO_STDOUT
_LIBCPP_HAS_NO_THREADS
_LIBCPP_HAS_NO_TIME_ZONE_DATABASE
_LIBCPP_HAS_NO_UNICODE
_LIBCPP_HAS_NO_VENDOR_AVAILABILITY_ANNOTATIONS

One common option I see being set is _LIBCPP_HAS_NO_THREADS, but that isn't being changed here. Did you miss that one, or is intentional that only a subset of the HAS_NO vars are being changed?

It looks like an oversight on @philnik777 's part. It also seems incorrect for users to be setting that, since this should be done via __config_site at configuration-time of the library.

It looks like all the usage is in build macros to generate a __config_site file.

@philnik777
Copy link
Contributor Author

As I said in the commit message, this only changes FTMs configured in <__config>. <__config_site> will be a follow-up patch. Re. some of the macros:

_LIBCPP_HAS_NO_FILESYSTEM_LIBRARY
_LIBCPP_HAS_NO_STDIN
_LIBCPP_HAS_NO_STDOUT

We don't use these macro names anymore, so any code that checks the macro is already broken.

_LIBCPP_HAS_NO_LIBRARY_ALIGNED_ALLOCATION
_LIBCPP_HAS_NO_VENDOR_AVAILABILITY_ANNOTATIONS
_LIBCPP_HAS_NO_INT128

I don't know why anybody would ever want to define that. Maybe some workaround for a bug?

_LIBCPP_HAS_NO_LOCALIZATION
_LIBCPP_HAS_NO_THREADS
_LIBCPP_HAS_NO_TIME_ZONE_DATABASE
_LIBCPP_HAS_NO_UNICODE

I'd be really interested what the reasoning for these is. Maybe not properly configured libc++ builds and people try to work around it?

_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER

I don't plan to change that one, but it should really only be set by our tests. OTOH I don't really care if people can't configure their warnings correctly because they define library-internal macros.

@iains
Copy link
Contributor

iains commented Apr 18, 2024

_LIBCPP_HAS_NO_VENDOR_AVAILABILITY_ANNOTATIONS

GCC does not implement the availability attribute (yet) I've been working on it - but it won't be in GCC-14 - so this would potentially affect GCC on Apple platforms.

_LIBCPP_HAS_NO_INT128

No 32b targets left?

@philnik777
Copy link
Contributor Author

_LIBCPP_HAS_NO_VENDOR_AVAILABILITY_ANNOTATIONS

GCC does not implement the availability attribute (yet) I've been working on it - but it won't be in GCC-14 - so this would potentially affect GCC on Apple platforms.

_LIBCPP_HAS_NO_INT128

No 32b targets left?

Availability annotations are disabled automatically on non-Clang compilers and __int128 availability is also detected through __SIZEOF_INT128__. There should be no need to set them explicitly.

@philnik777 philnik777 requested a review from a team as a code owner April 20, 2024 08:59
@llvmbot llvmbot added the libc++abi libc++abi C++ Runtime Library. Not libc++. label Apr 20, 2024
@philnik777 philnik777 force-pushed the always_define_internal_ftms branch 2 times, most recently from 72bd8d4 to 96bc5d5 Compare April 24, 2024 09:19
@ldionne
Copy link
Member

ldionne commented Apr 24, 2024

Oh, I was only looking at build configurations, I completely forgot about that. I see quite a bit more when I expand the scope for things like that, e.g. https://github.com/abseil/abseil-cpp/blob/192e959b16809f751d565b53a949b21129d904fb/absl/hash/internal/hash.h#L73

This specific usage looks wrong. I think it was a workaround for the fact that feature-test macros didn't take into account availability on the deployment target, but this has been fixed now. Checking for the feature-test macro only should be sufficient.

@philnik777 philnik777 force-pushed the always_define_internal_ftms branch 3 times, most recently from 95b5825 to a9b419b Compare May 2, 2024 20:37
@EricWF
Copy link
Member

EricWF commented May 2, 2024

I have a preference for the existing mechanisms. They've not caused my any trouble in my time on the project.

I think the existing mechanisms encourage/enforce the use of negative feature test macro names (ex. _LIBCPP_HAS_NO_EXCEPTIONS). I believe this naming scheme helps avoid misspelling/misconfiguration bugs,
since a misspelled guard will almost certainly cause a compilation failure. I worry the proposed approach isn't as robust against this.

This would change very long standing existing practice, which according to my recollection hasn't caused any major problems. Generally the macros are specified in the negative form (ex. _LIBCPP_HAS_NO_EXCEPTIONS), so they're only defined when a feature isn't available.

Because the current practice is so long standing, I would like this change to come with a clang-tidy that looked for misapplication of #ifdef to libc++ macros.

@EricWF EricWF self-requested a review May 2, 2024 21:30
@philnik777
Copy link
Contributor Author

I have a preference for the existing mechanisms. They've not caused my any trouble in my time on the project.

I think the existing mechanisms encourage/enforce the use of negative feature test macro names (ex. _LIBCPP_HAS_NO_EXCEPTIONS). I believe this naming scheme helps avoid misspelling/misconfiguration bugs, since a misspelled guard will almost certainly cause a compilation failure. I worry the proposed approach isn't as robust against this.

I'm not sure how the current mechanism is more robust. A misspelling might not be caught other than by testing the actual code (eg. #ifdef _LIBCPP_HAS_NO_EXCEPTION misspelled). The new mechanism will catch all misspellings at compile time (possibly even in your editor), since we use -Werror -Wundef. As I said in the description, a very similar change in <__availability> caught real bugs.

This would change very long standing existing practice, which according to my recollection hasn't caused any major problems. Generally the macros are specified in the negative form (ex. _LIBCPP_HAS_NO_EXCEPTIONS), so they're only defined when a feature isn't available.

Because the current practice is so long standing, I would like this change to come with a clang-tidy that looked for misapplication of #ifdef to libc++ macros.

While the practice is long-standing, the names of the individual macros changed before (cf65d27, b22aa3d, 5c40c99, 66a562d), without any major fallout I'm aware of. So I don't think a clang-tidy check pulls its weight here. A general check for internal macros might be nice, but is definitely out of scope for this change.

@EricWF
Copy link
Member

EricWF commented May 3, 2024

I have a preference for the existing mechanisms. They've not caused my any trouble in my time on the project.
I think the existing mechanisms encourage/enforce the use of negative feature test macro names (ex. _LIBCPP_HAS_NO_EXCEPTIONS). I believe this naming scheme helps avoid misspelling/misconfiguration bugs, since a misspelled guard will almost certainly cause a compilation failure. I worry the proposed approach isn't as robust against this.

I'm not sure how the current mechanism is more robust. A misspelling might not be caught other than by testing the actual code (eg. #ifdef _LIBCPP_HAS_NO_EXCEPTION misspelled).

Yes, like with -Wundef you need to compile the code.

That said, lets say had

#ifndef _LIBCPP_HAS_NO_EXXXXCEPTIONS
throw 42;
#endif

This gets caught because the code fails to compile. If the code successfully compiles, that means there's some other bugs going on that needs fixing. So it catches both misspellings and misconfigurations.

Note that the negative naming of the macro matters here. It's And is important even with the new approach.

With this change, we open up a bunch of new failure modes.

I'm more ok with changing to 1 or 0 than I am removing the negative feature test names. Did the <__availability> bugs you referenced use negative feature test names? Those are important, and I would consider blocking this change if it meant using positive feature tests.

Let me know if I need to do a better job of explaining why positive names are less desirable than negative ones.

As I said in the description, a very similar change in <__availability> caught real bugs.

Would you mind referencing the change?

This would change very long standing existing practice, which according to my recollection hasn't caused any major problems. Generally the macros are specified in the negative form (ex. _LIBCPP_HAS_NO_EXCEPTIONS), so they're only defined when a feature isn't available.
Because the current practice is so long standing, I would like this change to come with a clang-tidy that looked for misapplication of #ifdef to libc++ macros.

While the practice is long-standing, the names of the individual macros changed before (cf65d27, b22aa3d, 5c40c99, 66a562d), without any major fallout I'm aware of. So I don't think a clang-tidy check pulls its weight here. A general check for internal macros might be nice, but is definitely out of scope for this change.

You're renaming the macros because macro usage is bug prone. I'm don't agree with the claim, and I don't think it's out of scope, given that this change is about catching macro bugs and is otherwise NFC.

Let me know how you would like to proceed.

@ldionne
Copy link
Member

ldionne commented May 9, 2024

I'm more ok with changing to 1 or 0 than I am removing the negative feature test names. Did the <__availability> bugs you referenced use negative feature test names? Those are important, and I would consider blocking this change if it meant using positive feature tests.

Let me know if I need to do a better job of explaining why positive names are less desirable than negative ones.

I would like to better understand this aspect. My understanding is that we've been using negative "configuration macros" mainly because when we use #ifdef as a branching mechanism, it's more natural that the state where nothing is defined is the default configuration of the library. And since most (but not all) of our "configuration macros" are removing things from the default configuration (e.g. -fno-exceptions, -fno-rtti, etc), it makes sense that these macros would be negative.

However, note that this pattern doesn't always make sense. For example, @philnik777 pointed out to me that we have _LIBCPP_HAS_NO_ASAN. Our default configuration of the library doesn't assume ASAN, and so IMO it looks weird that our default configuration would be what's inside #ifndef _LIBCPP_HAS_NO_ASAN. Instead, it would make more sense to say #ifdef _LIBCPP_HAS_ASAN.

This patch basically puts all the configuration macros on the "same level" and uses a consistent mechanism to handle all of them.

There's still the question of whether we want to have

#if !_LIBCPP_HAS_NO_EXCEPTIONS
...
#endif

versus

#if _LIBCPP_HAS_EXCEPTIONS
...
#endif

IMO, avoiding the double-negative in this case makes things a lot easier to understand.

To summarize: I agree that negative macros make more sense in a #ifndef world, however I think their benefit is lost when we move to 0-1 macros instead. I'd love to know your thoughts on that.

@zibi2
Copy link
Contributor

zibi2 commented May 9, 2024

IMO, avoiding the double-negative in this case makes things a lot easier to understand.

I agree, I don't like double negation. It's hard to read. If the change makes our lives easier we should do it.

@EricWF
Copy link
Member

EricWF commented May 9, 2024

I agree with most of what @ldionne said. There in benefit to the negative macro formulation that is lost.
That benefit is making it harder to accidentally misuse the macro in a way that hides its misuse.

This can be mitigated by testing, and by tooling. If we're going to go this path, I think we should change the naming scheme to remove _LIBCPP_HAS. These sorts of names have a history of being defined/undefined, but never defined to 0. To avoid confusion, I think switching to a different naming scheme (any different naming system), would be valuable.

I might suggest _LIBCPP_FEATURE_ ?

@EricWF
Copy link
Member

EricWF commented May 9, 2024

While the practice is long-standing, the names of the individual macros changed before (cf65d27, b22aa3d, 5c40c99, 66a562d), without any major fallout I'm aware of. So I don't think a clang-tidy check pulls its weight here. A general check for internal macros might be nice, but is definitely out of scope for this change.

I don't believe any of those changes changed the way the macro works. They simply re-named things. So I wouldn't expect them to have "fallout" in the way I'm suggesting.

Also, if the purpose of this change is entirely for bug prevention & macro misuse, then a tool to assist with that is well in scope.

@ldionne
Copy link
Member

ldionne commented Sep 10, 2024

In light of the discussion we had in the libc++ monthly, I think we could update this patch. @philnik777

@philnik777 philnik777 force-pushed the always_define_internal_ftms branch 2 times, most recently from d2c09d2 to b65c0ef Compare September 15, 2024 10:45
Copy link
Member

@ldionne ldionne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This basically LGTM with a few comments. Let's try to land this soon to avoid many merge conflicts. I'd still like to have another look once the comments have been addressed.

This simplifies some complicated conditionals so much.

libcxx/include/__config Outdated Show resolved Hide resolved
libcxx/test/tools/clang_tidy_checks/internal_ftm_use.cpp Outdated Show resolved Hide resolved
libcxx/include/__config Outdated Show resolved Hide resolved
libcxx/include/__config Outdated Show resolved Hide resolved
libcxx/include/__configuration/availability.h Show resolved Hide resolved
libcxx/include/new Show resolved Hide resolved
libcxx/test/support/test_macros.h Outdated Show resolved Hide resolved
libcxx/test/support/test_macros.h Outdated Show resolved Hide resolved
libcxx/test/support/test_macros.h Outdated Show resolved Hide resolved
@philnik777 philnik777 force-pushed the always_define_internal_ftms branch 3 times, most recently from cc8658f to a8c8d14 Compare October 9, 2024 06:17
libcxx/include/__config Outdated Show resolved Hide resolved
libcxx/include/__memory/addressof.h Outdated Show resolved Hide resolved
@@ -98,7 +100,7 @@ inline _LIBCPP_HIDE_FROM_ABI _ValueType __libcpp_acquire_load(_ValueType const*

template <class _Tp>
inline _LIBCPP_HIDE_FROM_ABI _Tp __libcpp_atomic_refcount_increment(_Tp& __t) _NOEXCEPT {
#if defined(_LIBCPP_HAS_BUILTIN_ATOMIC_SUPPORT) && !defined(_LIBCPP_HAS_NO_THREADS)
#if _LIBCPP_HAS_BUILTIN_ATOMIC_SUPPORT && !defined(_LIBCPP_HAS_NO_THREADS)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to very quickly switch over to the same scheme for _LIBCPP_HAS_NO_THREADS and other things in __config_site, because that will become really confusing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll work on it as soon as this is landed.

@philnik777 philnik777 merged commit ba87515 into llvm:main Oct 12, 2024
10 of 12 checks passed
@philnik777 philnik777 deleted the always_define_internal_ftms branch October 12, 2024 07:49
DanielCChen pushed a commit to DanielCChen/llvm-project that referenced this pull request Oct 16, 2024
Currently, the library-internal feature test macros are only defined if
the feature is not available, and always have the prefix
`_LIBCPP_HAS_NO_`. This patch changes that, so that they are always
defined and have the prefix `_LIBCPP_HAS_` instead. This changes the
canonical use of these macros to `#if _LIBCPP_HAS_FEATURE`, which means
that using an undefined macro (e.g. due to a missing include) is
diagnosed now. While this is rather unlikely currently, a similar change
in `<__configuration/availability.h>` caught a few bugs. This also
improves readability, since it removes the double-negation of `#ifndef
_LIBCPP_HAS_NO_FEATURE`.

The current patch only touches the macros defined in `<__config>`. If
people are happy with this approach, I'll make a follow-up PR to also
change the macros defined in `<__config_site>`.
bricknerb pushed a commit to bricknerb/llvm-project that referenced this pull request Oct 17, 2024
Currently, the library-internal feature test macros are only defined if
the feature is not available, and always have the prefix
`_LIBCPP_HAS_NO_`. This patch changes that, so that they are always
defined and have the prefix `_LIBCPP_HAS_` instead. This changes the
canonical use of these macros to `#if _LIBCPP_HAS_FEATURE`, which means
that using an undefined macro (e.g. due to a missing include) is
diagnosed now. While this is rather unlikely currently, a similar change
in `<__configuration/availability.h>` caught a few bugs. This also
improves readability, since it removes the double-negation of `#ifndef
_LIBCPP_HAS_NO_FEATURE`.

The current patch only touches the macros defined in `<__config>`. If
people are happy with this approach, I'll make a follow-up PR to also
change the macros defined in `<__config_site>`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
libc++abi libc++abi C++ Runtime Library. Not libc++. libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants