[libcxx] Fix allocator<void>::pointer in C++20 with removed members

[ilya-biryukov]

Summary

When compiled with -D_LIBCPP_ENABLE_CXX20_REMOVED_ALLOCATOR_MEMBERS
uses of allocator<void>::pointer resulted in compiler errors after D104323.
If we instantiate the primary template, allocator<void>::reference produces
an error 'cannot form references to void'.

To workaround this, allow to bring back the allocator<void> specialization by defining the new _LIBCPP_ENABLE_CXX20_REMOVED_ALLOCATOR_VOID_SPECIALIZATION macro.

To make sure the code that uses allocator<void> and the removed members does not break,
both _LIBCPP_ENABLE_CXX20_REMOVED_ALLOCATOR_MEMBERS and _LIBCPP_ENABLE_CXX20_REMOVED_ALLOCATOR_MEMBERS have to be defined.

[philnik]

libcxx/include/__memory/allocator.h

30

I think using _LIBCPP_ENABLE_CXX20_REMOVED_ALLOCATOR_MEMBERS is misleading. This doesn't just re-enable members, but an entire class specialization. Adding something like _LIBCPP_ENABLE_CXX20_REMOVED_ALLOCATOR_VOID_SPECIALIZATION would be the correct way IMO.

libcxx/test/std/utilities/memory/default.allocator/allocator_types.void.compile.cxx20_with_removed_members.pass.cpp

1

This is a libc++ extension, so it should go in libcxx/test/libcxx and not libcxx/test/std.

18–23

You can just make this a .compile.pass.cpp and remove the main().

[ilya-biryukov]

libcxx/include/__memory/allocator.h

30

That keeps allocator<void>::pointer broken with _LIBCPP_ENABLE_CXX20_REMOVED_ALLOCATOR_MEMBERS defined and _LIBCPP_ENABLE_CXX20_REMOVED_ALLOCATOR_VOID_SPECIALIZATION not defined. Are we okay with that?

This specialization was there in the REMOVED_ALLOCATOR_MEMBERS mode before D104323 too, this change restores this behavior.

[philnik]

libcxx/include/__memory/allocator.h

30

I think it's Ok to have two flags for this, since the two changes are orthogonal IMO. But let's see what @ldionne thinks.

[ilya-biryukov]

• Move test to test/libcxx
• Switch test to 'compile.pass' and remove 'main'

[ilya-biryukov]

@ldionne friendly ping. Just want to make sure this does not fall off your radar.
Let me know if you need more time, no rush here from our side.

[ldionne]

In D126210#3537030, @ilya-biryukov wrote:

@ldionne friendly ping. Just want to make sure this does not fall off your radar.
Let me know if you need more time, no rush here from our side.

Thanks for the ping.

Let's also add a release note and document the option in libcxx/docs/UsingLibcxx.rst.

libcxx/include/__memory/allocator.h

30

Yes, I agree with using _LIBCPP_ENABLE_CXX20_REMOVED_ALLOCATOR_VOID_SPECIALIZATION for this.

[ilya-biryukov]

• Add _LIBCPP_ENABLE_CXX20_REMOVED_ALLOCATOR_VOID_SPECIALIZATION
• Document the new MACRO and add a release note

[ilya-biryukov]

Thanks for the quick response. I have addressed the comments, please take another look.

[ldionne]

libcxx/test/libcxx/utilities/memory/default.allocator/allocator_types.void.cxx20_allocator_void_no_members.fail.cpp

9–10 ↗

(On Diff #432025)

Can you explain what this test does? This sentence seems to be copy-pasted from the below test, but they are not checking the same thing.

[ilya-biryukov]

libcxx/test/libcxx/utilities/memory/default.allocator/allocator_types.void.cxx20_allocator_void_no_members.fail.cpp

9–10 ↗

(On Diff #432025)

It is copy-pasted indeed, thanks for catching that. I'll update the comment.
This test attempts to verify that the void specialization actually does not contain the removed members if the _LIBCPP_ENABLE_CXX20_REMOVED_ALLOCATOR_MEMBERS is not specified.

[ilya-biryukov]

• Update test comment, only check in C++20 and beyond, add expected-errors

[ilya-biryukov]

I've update change description and the added test. This should fix failures on buildbots and address the comments.
PTAL.

Side question: could you also point me to the docs that show how to run the test with clang in -verify mode?
I have tried passing --param=cxx_under_test=/usr/bin/clang++ to lit, but that did not even change the compiler that runs under tests.
I ended up running clang directly on the test with -Xclang -verify, but that's definitely not how other people do it :)

[ilya-biryukov]

• Fix test failure

[ilya-biryukov]

Answering my own question here: I ended up pointing CC and CXX to Clang instead of GCC when running CMake. This makes lit add the -verify flag to tests.

[ilya-biryukov]

@ldionne @philnik friendly ping, could you please take another look?

[ilya-biryukov]

@ldionne @philnik another friendly ping, please take a look when you have a spare minute.

[philnik]

LGTM % nit.

libcxx/test/libcxx/utilities/memory/default.allocator/allocator_types.void.cxx20_allocator_void_no_members.fail.cpp

1 ↗

(On Diff #433710)

Could you make this a .verify.cpp?

[ldionne]

LGTM with some comments. Please pay attention to the suggestions, some of them contain more than one fix per line and it's easy to miss!

Thanks!

libcxx/docs/ReleaseNotes.rst

112–116 ↗

(On Diff #433710)

libcxx/docs/UsingLibcxx.rst

328–330 ↗

(On Diff #433710)

libcxx/test/libcxx/utilities/memory/default.allocator/allocator_types.void.cxx20_allocator_void_no_members.fail.cpp

10 ↗

(On Diff #433710)

[ilya-biryukov]

• Change test from .fail.cpp to .verify.cpp

[ilya-biryukov]

• Fix typos and apply suggestions in documentation and comments

libcxx/include/__memory/allocator.h

 #if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
 #  pragma GCC system_header
 #endif
 
 _LIBCPP_BEGIN_NAMESPACE_STD
 
 template <class _Tp> class allocator;
 
-#if _LIBCPP_STD_VER <= 17
+#if _LIBCPP_STD_VER <= 17 || defined(_LIBCPP_ENABLE_CXX20_REMOVED_ALLOCATOR_MEMBERS)
 template <>
 class _LIBCPP_TEMPLATE_VIS allocator<void>
 {
 public:
     _LIBCPP_DEPRECATED_IN_CXX17 typedef void*             pointer;
     _LIBCPP_DEPRECATED_IN_CXX17 typedef const void*       const_pointer;
     _LIBCPP_DEPRECATED_IN_CXX17 typedef void              value_type;
 

libcxx/test/std/utilities/memory/default.allocator/allocator_types.void.compile.cxx20_with_removed_members.pass.cpp

+//===----------------------------------------------------------------------===//
+//
+// 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
+//
+//===----------------------------------------------------------------------===//
+
+// Check that the nested types of std::allocator<void> are provided in C++20
+// with a flag that keeps the removed members.
+
+// ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_DISABLE_DEPRECATION_WARNINGS
+// ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_ENABLE_CXX20_REMOVED_ALLOCATOR_MEMBERS
+
+#include <memory>
+#include <type_traits>
+
+int main() {
+  static_assert((std::is_same<std::allocator<void>::pointer, void*>::value), "");
+  static_assert((std::is_same<std::allocator<void>::const_pointer, const void*>::value), "");
+  static_assert((std::is_same<std::allocator<void>::value_type, void>::value), "");
+  static_assert((std::is_same<std::allocator<void>::rebind<int>::other, std::allocator<int> >::value), "");
+}