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++] P2641R4: Checking if a union alternative is active (std::is_within_lifetime) #107450

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

MitalAshok
Copy link
Contributor

@MitalAshok MitalAshok commented Sep 5, 2024

https://wg21.link/P2641R4

Implements the C++26 function in <type_traits> [meta.const.eval] (and the corresponding feature test macro __cpp_lib_is_within_lifetime)

template<class T>
  consteval bool is_within_lifetime(const T*) noexcept;

This is done with the __builtin_is_within_lifetime builtin added to Clang 20 by #91895 / 2a07509. This is not (currently) available with GCC.

This implementation has provisions for LWG4138 https://cplusplus.github.io/LWG/issue4138 where it is ill-formed to instantiate is_within_lifetime<T> with a function type T.

Closes #105381

@MitalAshok
Copy link
Contributor Author

GCC is expected to use the same name as Clang: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110367#c2

https://cplusplus.github.io/LWG/issue4138 is still open and the current proposed resolution doesn't allow void pointers either.

@MitalAshok MitalAshok marked this pull request as ready for review September 6, 2024 06:59
@MitalAshok MitalAshok requested a review from a team as a code owner September 6, 2024 06:59
@llvmbot llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Sep 6, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented Sep 6, 2024

@llvm/pr-subscribers-libcxx

Author: Mital Ashok (MitalAshok)

Changes

<https://wg21.link/P2641R4>

Implements the C++26 function in &lt;type_traits&gt; [meta.const.eval] (and the corresponding feature test macro __cpp_lib_is_within_lifetime)

template&lt;class T&gt;
  consteval bool is_within_lifetime(const T*) noexcept;

This is done with the __builtin_is_within_lifetime builtin added to Clang 20 by #91895 / 2a07509. This is not (currently) available with GCC or MSVC.

This implementation has provisions for LWG4138 <https://cplusplus.github.io/LWG/issue4138> where it ill-formed to instantiate the function definition where the argument would be a function pointer type.

Closes #105381


Full diff: https://github.com/llvm/llvm-project/pull/107450.diff

14 Files Affected:

  • (modified) libcxx/docs/FeatureTestMacroTable.rst (+1-1)
  • (modified) libcxx/docs/ReleaseNotes/20.rst (+1)
  • (modified) libcxx/docs/Status/Cxx2cPapers.csv (+1-1)
  • (modified) libcxx/include/CMakeLists.txt (+1)
  • (added) libcxx/include/__type_traits/is_within_lifetime.h (+36)
  • (modified) libcxx/include/module.modulemap (+1)
  • (modified) libcxx/include/type_traits (+8)
  • (modified) libcxx/include/version (+3-1)
  • (modified) libcxx/modules/std/type_traits.inc (+3)
  • (added) libcxx/test/libcxx/utilities/meta/is_within_lifetime.verify.cpp (+32)
  • (modified) libcxx/test/std/language.support/support.limits/support.limits.general/type_traits.version.compile.pass.cpp (+3-3)
  • (modified) libcxx/test/std/language.support/support.limits/support.limits.general/version.version.compile.pass.cpp (+3-3)
  • (added) libcxx/test/std/utilities/meta/meta.const.eval/is_within_lifetime.compile.pass.cpp (+119)
  • (modified) libcxx/utils/generate_feature_test_macro_components.py (+2-1)
diff --git a/libcxx/docs/FeatureTestMacroTable.rst b/libcxx/docs/FeatureTestMacroTable.rst
index c909a4300db1a6..ba927b36a44b58 100644
--- a/libcxx/docs/FeatureTestMacroTable.rst
+++ b/libcxx/docs/FeatureTestMacroTable.rst
@@ -446,7 +446,7 @@ Status
     ---------------------------------------------------------- -----------------
     ``__cpp_lib_is_virtual_base_of``                           ``202406L``
     ---------------------------------------------------------- -----------------
-    ``__cpp_lib_is_within_lifetime``                           *unimplemented*
+    ``__cpp_lib_is_within_lifetime``                           ``202306L``
     ---------------------------------------------------------- -----------------
     ``__cpp_lib_linalg``                                       *unimplemented*
     ---------------------------------------------------------- -----------------
diff --git a/libcxx/docs/ReleaseNotes/20.rst b/libcxx/docs/ReleaseNotes/20.rst
index 93d6027291ad95..c74181b9416c33 100644
--- a/libcxx/docs/ReleaseNotes/20.rst
+++ b/libcxx/docs/ReleaseNotes/20.rst
@@ -41,6 +41,7 @@ Implemented Papers
 - P2747R2: ``constexpr`` placement new (`Github <https://github.com/llvm/llvm-project/issues/105427>`__)
 - P2609R3: Relaxing Ranges Just A Smidge (`Github <https://github.com/llvm/llvm-project/issues/105253>`__)
 - P2985R0: A type trait for detecting virtual base classes (`Github <https://github.com/llvm/llvm-project/issues/105432>`__)
+- P2641R4: Checking if a ``union`` alternative is active (`Github <https://github.com/llvm/llvm-project/issues/105381>`__)
 
 
 Improvements and New Features
diff --git a/libcxx/docs/Status/Cxx2cPapers.csv b/libcxx/docs/Status/Cxx2cPapers.csv
index 8864b1ebe28891..c09fc6752040d7 100644
--- a/libcxx/docs/Status/Cxx2cPapers.csv
+++ b/libcxx/docs/Status/Cxx2cPapers.csv
@@ -18,7 +18,7 @@
 "`P2874R2 <https://wg21.link/P2874R2>`__","Mandating Annex D Require No More","2023-06 (Varna)","","",""
 "`P2757R3 <https://wg21.link/P2757R3>`__","Type-checking format args","2023-06 (Varna)","","",""
 "`P2637R3 <https://wg21.link/P2637R3>`__","Member ``visit``","2023-06 (Varna)","|Complete|","19.0",""
-"`P2641R4 <https://wg21.link/P2641R4>`__","Checking if a ``union`` alternative is active","2023-06 (Varna)","","",""
+"`P2641R4 <https://wg21.link/P2641R4>`__","Checking if a ``union`` alternative is active","2023-06 (Varna)","|Complete|","20.0",""
 "`P1759R6 <https://wg21.link/P1759R6>`__","Native handles and file streams","2023-06 (Varna)","|Complete|","18.0",""
 "`P2697R1 <https://wg21.link/P2697R1>`__","Interfacing ``bitset`` with ``string_view``","2023-06 (Varna)","|Complete|","18.0",""
 "`P1383R2 <https://wg21.link/P1383R2>`__","More ``constexpr`` for ``<cmath>`` and ``<complex>``","2023-06 (Varna)","","",""
diff --git a/libcxx/include/CMakeLists.txt b/libcxx/include/CMakeLists.txt
index 0f43916dae4384..e47a489da3d966 100644
--- a/libcxx/include/CMakeLists.txt
+++ b/libcxx/include/CMakeLists.txt
@@ -822,6 +822,7 @@ set(files
   __type_traits/is_valid_expansion.h
   __type_traits/is_void.h
   __type_traits/is_volatile.h
+  __type_traits/is_within_lifetime.h
   __type_traits/lazy.h
   __type_traits/make_32_64_or_128_bit.h
   __type_traits/make_const_lvalue_ref.h
diff --git a/libcxx/include/__type_traits/is_within_lifetime.h b/libcxx/include/__type_traits/is_within_lifetime.h
new file mode 100644
index 00000000000000..363f9c922a9076
--- /dev/null
+++ b/libcxx/include/__type_traits/is_within_lifetime.h
@@ -0,0 +1,36 @@
+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef _LIBCPP___TYPE_TRAITS_IS_WITHIN_LIFETIME_H
+#define _LIBCPP___TYPE_TRAITS_IS_WITHIN_LIFETIME_H
+
+#include <__config>
+#include <__type_traits/is_function.h>
+
+#if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
+#  pragma GCC system_header
+#endif
+
+_LIBCPP_BEGIN_NAMESPACE_STD
+
+#if _LIBCPP_STD_VER >= 26 && __has_builtin(__builtin_is_within_lifetime)
+template <class _Tp>
+_LIBCPP_HIDE_FROM_ABI inline consteval bool is_within_lifetime(const _Tp* __p) noexcept {
+  if constexpr (is_function_v<_Tp>) {
+    // Avoid multiple diagnostics
+    static_assert(!is_function_v<_Tp>, "std::is_within_lifetime<T> cannot explicitly specify T as a function type");
+    return true;
+  } else {
+    return __builtin_is_within_lifetime(__p);
+  }
+}
+#endif
+
+_LIBCPP_END_NAMESPACE_STD
+
+#endif // _LIBCPP___TYPE_TRAITS_IS_WITHIN_LIFETIME_H
diff --git a/libcxx/include/module.modulemap b/libcxx/include/module.modulemap
index 3abc11723a5a92..f175bf91dc5c53 100644
--- a/libcxx/include/module.modulemap
+++ b/libcxx/include/module.modulemap
@@ -2008,6 +2008,7 @@ module std_private_type_traits_is_void                                   [system
   export std_private_type_traits_integral_constant
 }
 module std_private_type_traits_is_volatile                               [system] { header "__type_traits/is_volatile.h" }
+module std_private_type_traits_is_within_lifetime                        [system] { header "__type_traits/is_within_lifetime.h" }
 module std_private_type_traits_lazy                                      [system] { header "__type_traits/lazy.h" }
 module std_private_type_traits_make_32_64_or_128_bit                     [system] { header "__type_traits/make_32_64_or_128_bit.h" }
 module std_private_type_traits_make_const_lvalue_ref                     [system] { header "__type_traits/make_const_lvalue_ref.h" }
diff --git a/libcxx/include/type_traits b/libcxx/include/type_traits
index 5937d4fdc9e1a7..39e22808e6ea2a 100644
--- a/libcxx/include/type_traits
+++ b/libcxx/include/type_traits
@@ -416,6 +416,10 @@ namespace std
       template<class B>
         inline constexpr bool negation_v = negation<B>::value;           // C++17
 
+      // [meta.const.eval], constant evaluation context
+      constexpr bool is_constant_evaluated() noexcept;                   // C++20
+      template<class T>
+        consteval bool is_within_lifetime(const T*) noexcept;            // C++26
 }
 
 */
@@ -517,6 +521,10 @@ namespace std
 #  include <__type_traits/unwrap_ref.h>
 #endif
 
+#if _LIBCPP_STD_VER >= 26
+#  include <__type_traits/is_within_lifetime.h>
+#endif
+
 #include <version>
 
 #if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
diff --git a/libcxx/include/version b/libcxx/include/version
index dc1d3fd268ce83..d5679dc7bd73b4 100644
--- a/libcxx/include/version
+++ b/libcxx/include/version
@@ -539,7 +539,9 @@ __cpp_lib_void_t                                        201411L <type_traits>
 # if __has_builtin(__builtin_is_virtual_base_of)
 #   define __cpp_lib_is_virtual_base_of                 202406L
 # endif
-// # define __cpp_lib_is_within_lifetime                   202306L
+# if __has_builtin(__builtin_is_within_lifetime)
+#   define __cpp_lib_is_within_lifetime                 202306L
+# endif
 // # define __cpp_lib_linalg                               202311L
 # undef  __cpp_lib_mdspan
 # define __cpp_lib_mdspan                               202406L
diff --git a/libcxx/modules/std/type_traits.inc b/libcxx/modules/std/type_traits.inc
index 485a5ddf63aed0..fd9ea15349e1ce 100644
--- a/libcxx/modules/std/type_traits.inc
+++ b/libcxx/modules/std/type_traits.inc
@@ -314,6 +314,9 @@ export namespace std {
 
   // [meta.const.eval], constant evaluation context
   using std::is_constant_evaluated;
+#if _LIBCPP_STD_VER >= 26 && __has_builtin(__builtin_is_within_lifetime)
+  using std::is_within_lifetime;
+#endif
 
   // [depr.meta.types]
   using std::aligned_storage;
diff --git a/libcxx/test/libcxx/utilities/meta/is_within_lifetime.verify.cpp b/libcxx/test/libcxx/utilities/meta/is_within_lifetime.verify.cpp
new file mode 100644
index 00000000000000..eef926660756ba
--- /dev/null
+++ b/libcxx/test/libcxx/utilities/meta/is_within_lifetime.verify.cpp
@@ -0,0 +1,32 @@
+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+// UNSUPPORTED: c++03
+
+// <type_traits>
+
+// LWG4138 <https://cplusplus.github.io/LWG/issue4138>
+// std::is_within_lifetime shouldn't work when a function type is
+// explicitly specified, even if it isn't evaluated
+
+#include <type_traits>
+#include <cassert>
+
+#include "test_macros.h"
+
+void fn();
+
+int main(int, char**) {
+#ifdef __cpp_lib_is_within_lifetime
+  constexpr bool _ = true ? false : std::is_within_lifetime<void()>(&fn);
+  // expected-error@*:* {{static assertion failed due to requirement '!is_function_v<void ()>': std::is_within_lifetime<T> cannot explicitly specify T as a function type}}
+#else
+  // expected-no-diagnostics
+#endif
+  return 0;
+}
diff --git a/libcxx/test/std/language.support/support.limits/support.limits.general/type_traits.version.compile.pass.cpp b/libcxx/test/std/language.support/support.limits/support.limits.general/type_traits.version.compile.pass.cpp
index 1cbf2699a95bcc..22a1629637bd57 100644
--- a/libcxx/test/std/language.support/support.limits/support.limits.general/type_traits.version.compile.pass.cpp
+++ b/libcxx/test/std/language.support/support.limits/support.limits.general/type_traits.version.compile.pass.cpp
@@ -870,16 +870,16 @@
 #   endif
 # endif
 
-# if !defined(_LIBCPP_VERSION)
+# if __has_builtin(__builtin_is_within_lifetime)
 #   ifndef __cpp_lib_is_within_lifetime
 #     error "__cpp_lib_is_within_lifetime should be defined in c++26"
 #   endif
 #   if __cpp_lib_is_within_lifetime != 202306L
 #     error "__cpp_lib_is_within_lifetime should have the value 202306L in c++26"
 #   endif
-# else // _LIBCPP_VERSION
+# else
 #   ifdef __cpp_lib_is_within_lifetime
-#     error "__cpp_lib_is_within_lifetime should not be defined because it is unimplemented in libc++!"
+#     error "__cpp_lib_is_within_lifetime should not be defined when the requirement '__has_builtin(__builtin_is_within_lifetime)' is not met!"
 #   endif
 # endif
 
diff --git a/libcxx/test/std/language.support/support.limits/support.limits.general/version.version.compile.pass.cpp b/libcxx/test/std/language.support/support.limits/support.limits.general/version.version.compile.pass.cpp
index a022c90e166c8d..83cdb172fbcdfb 100644
--- a/libcxx/test/std/language.support/support.limits/support.limits.general/version.version.compile.pass.cpp
+++ b/libcxx/test/std/language.support/support.limits/support.limits.general/version.version.compile.pass.cpp
@@ -7186,16 +7186,16 @@
 #   endif
 # endif
 
-# if !defined(_LIBCPP_VERSION)
+# if __has_builtin(__builtin_is_within_lifetime)
 #   ifndef __cpp_lib_is_within_lifetime
 #     error "__cpp_lib_is_within_lifetime should be defined in c++26"
 #   endif
 #   if __cpp_lib_is_within_lifetime != 202306L
 #     error "__cpp_lib_is_within_lifetime should have the value 202306L in c++26"
 #   endif
-# else // _LIBCPP_VERSION
+# else
 #   ifdef __cpp_lib_is_within_lifetime
-#     error "__cpp_lib_is_within_lifetime should not be defined because it is unimplemented in libc++!"
+#     error "__cpp_lib_is_within_lifetime should not be defined when the requirement '__has_builtin(__builtin_is_within_lifetime)' is not met!"
 #   endif
 # endif
 
diff --git a/libcxx/test/std/utilities/meta/meta.const.eval/is_within_lifetime.compile.pass.cpp b/libcxx/test/std/utilities/meta/meta.const.eval/is_within_lifetime.compile.pass.cpp
new file mode 100644
index 00000000000000..96d21249a499fe
--- /dev/null
+++ b/libcxx/test/std/utilities/meta/meta.const.eval/is_within_lifetime.compile.pass.cpp
@@ -0,0 +1,119 @@
+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+// UNSUPPORTED: c++03
+
+// <type_traits>
+
+// template <class T>
+//   consteval bool is_within_lifetime(const T*) noexcept; // C++26
+
+#include <type_traits>
+#include <cassert>
+
+#include "test_macros.h"
+
+#ifndef __cpp_lib_is_within_lifetime
+
+// Check that it doesn't exist if the feature test macro isn't defined (via ADL)
+template <class T>
+constexpr decltype(static_cast<void>(is_within_lifetime(std::declval<T>())), bool{}) is_within_lifetime_exists(int) {
+  return true;
+}
+template <class T>
+constexpr bool is_within_lifetime_exists(long) {
+  return false;
+}
+
+static_assert(!is_within_lifetime_exists<const std::integral_constant<bool, false>*>(0), "");
+
+#elif TEST_STD_VER < 26
+#  error __cpp_lib_is_within_lifetime defined before C++26
+#else // defined(__cpp_lib_is_within_lifetime) && TEST_STD_VER >= 26
+
+ASSERT_SAME_TYPE(decltype(std::is_within_lifetime(std::declval<int*>())), bool);
+ASSERT_SAME_TYPE(decltype(std::is_within_lifetime(std::declval<const int*>())), bool);
+ASSERT_SAME_TYPE(decltype(std::is_within_lifetime(std::declval<void*>())), bool);
+ASSERT_SAME_TYPE(decltype(std::is_within_lifetime(std::declval<const void*>())), bool);
+
+ASSERT_NOEXCEPT(std::is_within_lifetime(std::declval<int*>()));
+ASSERT_NOEXCEPT(std::is_within_lifetime(std::declval<const int*>()));
+ASSERT_NOEXCEPT(std::is_within_lifetime(std::declval<void*>()));
+ASSERT_NOEXCEPT(std::is_within_lifetime(std::declval<const void*>()));
+
+template <class T>
+concept is_within_lifetime_exists = requires(T t) { std::is_within_lifetime(t); };
+
+struct S {};
+
+static_assert(is_within_lifetime_exists<int*>);
+static_assert(is_within_lifetime_exists<const int*>);
+static_assert(is_within_lifetime_exists<void*>);
+static_assert(is_within_lifetime_exists<const void*>);
+static_assert(!is_within_lifetime_exists<int>);               // Not a pointer
+static_assert(!is_within_lifetime_exists<decltype(nullptr)>); // Not a pointer
+static_assert(!is_within_lifetime_exists<void() const>);      // Not a pointer
+static_assert(!is_within_lifetime_exists<int S::*>);          // Doesn't accept pointer-to-member
+static_assert(!is_within_lifetime_exists<void (S::*)()>);
+static_assert(!is_within_lifetime_exists<void (*)()>); // Doesn't match `const T*`
+
+constexpr int i = 0;
+static_assert(std::is_within_lifetime(&i));
+static_assert(std::is_within_lifetime(const_cast<int*>(&i)));
+static_assert(std::is_within_lifetime(static_cast<const void*>(&i)));
+static_assert(std::is_within_lifetime(static_cast<void*>(const_cast<int*>(&i))));
+static_assert(std::is_within_lifetime<const int>(&i));
+static_assert(std::is_within_lifetime<int>(const_cast<int*>(&i)));
+static_assert(std::is_within_lifetime<const void>(static_cast<const void*>(&i)));
+static_assert(std::is_within_lifetime<void>(static_cast<void*>(const_cast<int*>(&i))));
+
+constexpr union {
+  int active;
+  int inactive;
+} u{.active = 1};
+static_assert(std::is_within_lifetime(&u.active) && !std::is_within_lifetime(&u.inactive));
+
+consteval bool f() {
+  union {
+    int active;
+    int inactive;
+  };
+  if (std::is_within_lifetime(&active) || std::is_within_lifetime(&inactive))
+    return false;
+  active = 1;
+  if (!std::is_within_lifetime(&active) || std::is_within_lifetime(&inactive))
+    return false;
+  inactive = 1;
+  if (std::is_within_lifetime(&active) || !std::is_within_lifetime(&inactive))
+    return false;
+  int j;
+  S s;
+  return std::is_within_lifetime(&j) && std::is_within_lifetime(&s);
+}
+static_assert(f());
+
+#  ifndef TEST_COMPILER_MSVC
+// MSVC doesn't support consteval propagation :(
+template <typename T>
+constexpr void does_escalate(T p) {
+  std::is_within_lifetime(p);
+}
+
+template <typename T, void(T) = does_escalate<T>>
+constexpr bool check_escalated(int) {
+  return false;
+}
+template <typename T>
+constexpr bool check_escalated(long) {
+  return true;
+}
+static_assert(check_escalated<int*>(0), "");
+static_assert(check_escalated<void*>(0), "");
+#  endif // defined(TEST_COMPILER_MSVC)
+
+#endif // defined(__cpp_lib_is_within_lifetime) && TEST_STD_VER >= 26
diff --git a/libcxx/utils/generate_feature_test_macro_components.py b/libcxx/utils/generate_feature_test_macro_components.py
index 3bdd3adad15b4e..f6817b24dfbf1c 100755
--- a/libcxx/utils/generate_feature_test_macro_components.py
+++ b/libcxx/utils/generate_feature_test_macro_components.py
@@ -796,7 +796,8 @@ def add_version_header(tc):
                 "c++26": 202306  # P2641R4 Checking if a union alternative is active
             },
             "headers": ["type_traits"],
-            "unimplemented": True,
+            "test_suite_guard": "__has_builtin(__builtin_is_within_lifetime)",
+            "libcxx_guard": "__has_builtin(__builtin_is_within_lifetime)",
         },
         {
             "name": "__cpp_lib_jthread",

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.

Thanks for the patch! I have some comments about the form, but the patch itself makes sense to me.

//
//===----------------------------------------------------------------------===//

// UNSUPPORTED: c++03
Copy link
Member

Choose a reason for hiding this comment

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

Ditto for standard modes here.

libcxx/include/__type_traits/is_within_lifetime.h Outdated Show resolved Hide resolved
@MitalAshok
Copy link
Contributor Author

Thanks for your review! I have tried to address all of your comments

I've also run into the CI test runner running clang-19 and not clang-20, like here: #106870 (comment). These tests at least work on my machine.

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.

Sorry for the delay! LGTM with minor changes.

You'll have to rebase this on top of main, too. I suggest you discard your changes to the modulemap and re-apply them manually instead, that's a lot easier.

Comment on lines +24 to +28
if constexpr (is_function_v<_Tp>) {
// Avoid multiple diagnostics
static_assert(!is_function_v<_Tp>, "std::is_within_lifetime<T> cannot explicitly specify T as a function type");
return false;
} else {
Copy link
Member

Choose a reason for hiding this comment

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

This is clever, but I think I'd rather use the naive way we consistently do this, and just have static_assert at the start of the function. If we think the duplicate diagnostics are bad, that's something we can (and should) fix in Clang (e.g. when it hits a static_assert it should probably stop).

There have been past discussions about this and I'm not really ready to start working around the issue in the library, I don't think that's the right place to do this.

Copy link
Contributor

Choose a reason for hiding this comment

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

If the builtin already diagnoses this, why do we add the static_assert at all?

Copy link
Member

Choose a reason for hiding this comment

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

It would be possible for the builtin on other implementations not to produce a clear diagnostic, I guess. I'm mostly neutral on this FWIW.

Copy link
Contributor

Choose a reason for hiding this comment

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

If that's a problem we can still add a static assert and file a bug against GCC though. FWIW I've filed a bug for __builtin_launder against GCC some time ago and they agreed it's a bug and fixed it fairly quickly.

@@ -2008,6 +2008,7 @@ module std_private_type_traits_is_void [system
export std_private_type_traits_integral_constant
}
module std_private_type_traits_is_volatile [system] { header "__type_traits/is_volatile.h" }
module std_private_type_traits_is_within_lifetime [system] { header "__type_traits/is_within_lifetime.h" }
Copy link
Member

Choose a reason for hiding this comment

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

When you rebase onto main, I would recommend you discard the changes to the modulemap entirely and then re-add them again. It's going to be less confusing than resolving the merge conflict.

@ldionne
Copy link
Member

ldionne commented Oct 21, 2024

Gentle ping @MitalAshok, it looks like we're very close to being able to merge this :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

P2641R4: Checking if a union alternative is active
4 participants