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++] Deprecate the _LIBCPP_ENABLE_CXX20_REMOVED_ALLOCATOR_MEMBERS macro #77692

Merged
merged 4 commits into from
Jan 12, 2024

Conversation

ldionne
Copy link
Member

@ldionne ldionne commented Jan 10, 2024

As described in #69994, using the escape hatch makes us non-conforming in C++20 due to incorrect constexpr-ness. It also leads to bad diagnostics as reported by #63900. We discussed the issue in the libc++ monthly meeting and we agreed that we should deprecate the macro in LLVM 18, and then remove it in LLVM 19 since it causes too many problems.

This patch does the first part of this -- it deprecates the macro.

Fixes #69994
Fixes #63900
Partially addresses #75975

… macro

As described in llvm#69994, using the escape hatch makes us non-conforming
in C++20 due to incorrect constexpr-ness. It also leads to bad diagnostics
as reported by llvm#63900. We discussed the issue in the libc++ monthly meeting
and we agreed that we should deprecate the macro in LLVM 18, and then
remove it in LLVM 19 since it causes too many problems.

This patch does the first part of this -- it deprecates the macro.

Fixes llvm#69994
Fixes llvm#63900
Partially addresses llvm#75975
@ldionne ldionne requested a review from a team as a code owner January 10, 2024 22:03
@llvmbot llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Jan 10, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented Jan 10, 2024

@llvm/pr-subscribers-libcxx

Author: Louis Dionne (ldionne)

Changes

As described in #69994, using the escape hatch makes us non-conforming in C++20 due to incorrect constexpr-ness. It also leads to bad diagnostics as reported by #63900. We discussed the issue in the libc++ monthly meeting and we agreed that we should deprecate the macro in LLVM 18, and then remove it in LLVM 19 since it causes too many problems.

This patch does the first part of this -- it deprecates the macro.

Fixes #69994
Fixes #63900
Partially addresses #75975


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

5 Files Affected:

  • (modified) libcxx/docs/ReleaseNotes/18.rst (+9)
  • (modified) libcxx/include/__memory/allocator.h (+6)
  • (modified) libcxx/test/libcxx/depr/depr.default.allocator/allocator.members/address.depr_in_cxx17.verify.cpp (+1-1)
  • (modified) libcxx/test/libcxx/depr/depr.default.allocator/allocator.members/allocate.depr_in_cxx17.verify.cpp (+1-1)
  • (added) libcxx/test/libcxx/depr/depr.default.allocator/enable_removed_allocator_members.deprecated.verify.cpp (+18)
diff --git a/libcxx/docs/ReleaseNotes/18.rst b/libcxx/docs/ReleaseNotes/18.rst
index 5df6242e52317a..ced632022ee497 100644
--- a/libcxx/docs/ReleaseNotes/18.rst
+++ b/libcxx/docs/ReleaseNotes/18.rst
@@ -117,6 +117,12 @@ Deprecations and Removals
   and ``<experimental/vector>`` have been removed in LLVM 18, as all their contents will have been
   implemented in namespace ``std`` for at least two releases.
 
+- The macro ``_LIBCPP_ENABLE_CXX20_REMOVED_ALLOCATOR_MEMBERS`` has been deprecated and will be removed
+  in LLVM 19. This macro used to re-enable redundant members of ``std::allocator<T>`` like ``pointer``,
+  ``reference``, ``rebind``, ``address``, ``max_size``, ``construct``, ``destroy``, and the two-argument
+  overload of ``allocate``. However, this led to the library being non-conforming due to incorrect
+  constexpr-ness. As a result the macro is being deprecated and will be removed in LLVM 19.
+
 Upcoming Deprecations and Removals
 ----------------------------------
 
@@ -140,6 +146,9 @@ LLVM 19
 - The ``_LIBCPP_ENABLE_NARROWING_CONVERSIONS_IN_VARIANT`` macro that changed the behavior for narrowing conversions
   in ``std::variant`` will be removed in LLVM 19.
 
+- The ``_LIBCPP_ENABLE_CXX20_REMOVED_ALLOCATOR_MEMBERS`` macro has been deprecated in LLVM 18 and will be removed
+  entirely in LLVM 19.
+
 LLVM 20
 ~~~~~~~
 
diff --git a/libcxx/include/__memory/allocator.h b/libcxx/include/__memory/allocator.h
index 747ce30d8fef61..8d54e0f0897dda 100644
--- a/libcxx/include/__memory/allocator.h
+++ b/libcxx/include/__memory/allocator.h
@@ -31,6 +31,12 @@ _LIBCPP_BEGIN_NAMESPACE_STD
 template <class _Tp>
 class allocator;
 
+#if defined(_LIBCPP_ENABLE_CXX20_REMOVED_ALLOCATOR_MEMBERS) && !defined(_LIBCPP_DISABLE_DEPRECATION_WARNINGS)
+#  pragma clang deprecated(                                                                                            \
+      _LIBCPP_ENABLE_CXX20_REMOVED_ALLOCATOR_MEMBERS,                                                                  \
+      "_LIBCPP_ENABLE_CXX20_REMOVED_ALLOCATOR_MEMBERS is deprecated in LLVM 18 and will be removed in LLVM 19")
+#endif
+
 #if _LIBCPP_STD_VER <= 17 || defined(_LIBCPP_ENABLE_CXX20_REMOVED_ALLOCATOR_VOID_SPECIALIZATION)
 // These specializations shouldn't be marked _LIBCPP_DEPRECATED_IN_CXX17.
 // Specializing allocator<void> is deprecated, but not using it.
diff --git a/libcxx/test/libcxx/depr/depr.default.allocator/allocator.members/address.depr_in_cxx17.verify.cpp b/libcxx/test/libcxx/depr/depr.default.allocator/allocator.members/address.depr_in_cxx17.verify.cpp
index 5b21cc607711e9..83d059a838ffb7 100644
--- a/libcxx/test/libcxx/depr/depr.default.allocator/allocator.members/address.depr_in_cxx17.verify.cpp
+++ b/libcxx/test/libcxx/depr/depr.default.allocator/allocator.members/address.depr_in_cxx17.verify.cpp
@@ -16,7 +16,7 @@
 
 // UNSUPPORTED: c++03, c++11, c++14
 
-// ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_ENABLE_CXX20_REMOVED_ALLOCATOR_MEMBERS
+// ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_ENABLE_CXX20_REMOVED_ALLOCATOR_MEMBERS -Wno-deprecated-pragma
 
 #include <memory>
 
diff --git a/libcxx/test/libcxx/depr/depr.default.allocator/allocator.members/allocate.depr_in_cxx17.verify.cpp b/libcxx/test/libcxx/depr/depr.default.allocator/allocator.members/allocate.depr_in_cxx17.verify.cpp
index 79fd61d24c4cee..8b2e862e9503e9 100644
--- a/libcxx/test/libcxx/depr/depr.default.allocator/allocator.members/allocate.depr_in_cxx17.verify.cpp
+++ b/libcxx/test/libcxx/depr/depr.default.allocator/allocator.members/allocate.depr_in_cxx17.verify.cpp
@@ -15,7 +15,7 @@
 
 // UNSUPPORTED: c++03, c++11, c++14
 
-// ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_ENABLE_CXX20_REMOVED_ALLOCATOR_MEMBERS
+// ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_ENABLE_CXX20_REMOVED_ALLOCATOR_MEMBERS -Wno-deprecated-pragma
 
 #include <memory>
 
diff --git a/libcxx/test/libcxx/depr/depr.default.allocator/enable_removed_allocator_members.deprecated.verify.cpp b/libcxx/test/libcxx/depr/depr.default.allocator/enable_removed_allocator_members.deprecated.verify.cpp
new file mode 100644
index 00000000000000..5d79ad339a5024
--- /dev/null
+++ b/libcxx/test/libcxx/depr/depr.default.allocator/enable_removed_allocator_members.deprecated.verify.cpp
@@ -0,0 +1,18 @@
+//===----------------------------------------------------------------------===//
+//
+// 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
+//
+//===----------------------------------------------------------------------===//
+
+// <memory>
+
+// Ensure that defining _LIBCPP_ENABLE_CXX20_REMOVED_ALLOCATOR_MEMBERS yields a
+// deprecation warning. We intend to issue a deprecation warning in LLVM 18
+// and remove the macro entirely in LLVM 19. As such, this test will be quite
+// short lived.
+
+// ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_ENABLE_CXX20_REMOVED_ALLOCATOR_MEMBERS
+
+#include <memory> // expected-warning@* 1+ {{macro '_LIBCPP_ENABLE_CXX20_REMOVED_ALLOCATOR_MEMBERS' has been marked as deprecated}}

Copy link
Member

@mordante mordante left a comment

Choose a reason for hiding this comment

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

LGTM modulo one nit and when the CI is green.

libcxx/docs/ReleaseNotes/18.rst Outdated Show resolved Hide resolved
@ldionne ldionne merged commit 8751bbe into llvm:main Jan 12, 2024
44 checks passed
@ldionne ldionne deleted the review/deprecate-allocator-members-macro branch January 12, 2024 14:51
@jyknight
Copy link
Member

My suggestion on #69994 had been to stop implying _LIBCPP_ENABLE_CXX20_REMOVED_ALLOCATOR_MEMBERS from _LIBCPP_ENABLE_CXX20_REMOVED_FEATURES in LLVM 18 at the same time as deprecating it. Did you intend to not do that, or was it just missed?

@philnik777
Copy link
Contributor

My suggestion on #69994 had been to stop implying _LIBCPP_ENABLE_CXX20_REMOVED_ALLOCATOR_MEMBERS from _LIBCPP_ENABLE_CXX20_REMOVED_FEATURES in LLVM 18 at the same time as deprecating it. Did you intend to not do that, or was it just missed?

We've also deprecated (or plan to?) _LIBCPP_ENABLE_CXX20_REMOVED_FEATURES, so there isn't really a point in changing anything there now.

@jyknight
Copy link
Member

Ah, I see that now. SGTM, thanks for the clarification!

@mordante
Copy link
Member

My suggestion on #69994 had been to stop implying _LIBCPP_ENABLE_CXX20_REMOVED_ALLOCATOR_MEMBERS from _LIBCPP_ENABLE_CXX20_REMOVED_FEATURES in LLVM 18 at the same time as deprecating it. Did you intend to not do that, or was it just missed?

We've also deprecated (or plan to?) _LIBCPP_ENABLE_CXX20_REMOVED_FEATURES, so there isn't really a point in changing anything there now.

I've created #77879 for that yesterday.

justinfargnoli pushed a commit to justinfargnoli/llvm-project that referenced this pull request Jan 28, 2024
… macro (llvm#77692)

As described in llvm#69994, using the escape hatch makes us non-conforming
in C++20 due to incorrect constexpr-ness. It also leads to bad
diagnostics as reported by llvm#63900. We discussed the issue in the libc++
monthly meeting and we agreed that we should deprecate the macro in LLVM
18, and then remove it in LLVM 19 since it causes too many problems.

This patch does the first part of this -- it deprecates the macro.

Fixes llvm#69994
Fixes llvm#63900
Partially addresses llvm#75975
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
5 participants