Skip to content

Conversation

@StephanTLavavej
Copy link
Member

Found while running libc++'s test suite with MSVC's STL.

MSVC's STL rejects allocator<const T>. This may or may not be justified by the current Standardese (it was bogus in the C++03 era), but it's how we reject usage like vector<const T>.

A bunch of mdspan tests are failing for us because some centralized machinery is using allocator<const T>. Testing that mdspan and its associated types work properly with const T is good and necessary, but directly allocating const T is what's a problem for MSVC's STL. I'd like to ask for a very targeted change here that preserves all of the test coverage but changes how ElementPool interacts with allocator.

This intentionally leaves ElementPool::get_ptr() returning T* (pointer-to-possibly-const), so there's no externally visible difference.

This intentionally leaves `ElementPool::get_ptr()` returning `T*`, so there's no externally visible difference.
@StephanTLavavej StephanTLavavej requested a review from a team as a code owner November 27, 2023 17:02
@llvmbot llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Nov 27, 2023
@llvmbot
Copy link
Member

llvmbot commented Nov 27, 2023

@llvm/pr-subscribers-libcxx

Author: Stephan T. Lavavej (StephanTLavavej)

Changes

Found while running libc++'s test suite with MSVC's STL.

MSVC's STL rejects allocator&lt;const T&gt;. This may or may not be justified by the current Standardese (it was bogus in the C++03 era), but it's how we reject usage like vector&lt;const T&gt;.

A bunch of mdspan tests are failing for us because some centralized machinery is using allocator&lt;const T&gt;. Testing that mdspan and its associated types work properly with const T is good and necessary, but directly allocating const T is what's a problem for MSVC's STL. I'd like to ask for a very targeted change here that preserves all of the test coverage but changes how ElementPool interacts with allocator.

This intentionally leaves ElementPool::get_ptr() returning T* (pointer-to-possibly-const), so there's no externally visible difference.


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

1 Files Affected:

  • (modified) libcxx/test/std/containers/views/mdspan/MinimalElementType.h (+5-4)
diff --git a/libcxx/test/std/containers/views/mdspan/MinimalElementType.h b/libcxx/test/std/containers/views/mdspan/MinimalElementType.h
index b1fbd6ed944d179..fe7f0e1f2383790 100644
--- a/libcxx/test/std/containers/views/mdspan/MinimalElementType.h
+++ b/libcxx/test/std/containers/views/mdspan/MinimalElementType.h
@@ -9,7 +9,8 @@
 #ifndef TEST_STD_CONTAINERS_VIEWS_MDSPAN_MINIMAL_ELEMENT_TYPE_H
 #define TEST_STD_CONTAINERS_VIEWS_MDSPAN_MINIMAL_ELEMENT_TYPE_H
 
-#include<memory>
+#include <memory>
+#include <type_traits>
 
 // Idiosyncratic element type for mdspan
 // Make sure we don't assume copyable, default constructible, movable etc.
@@ -25,7 +26,7 @@ struct MinimalElementType {
 template<class T, size_t N>
 struct ElementPool {
   constexpr ElementPool() {
-    ptr_ = std::allocator<T>().allocate(N);
+    ptr_ = std::allocator<std::remove_const_t<T>>().allocate(N);
     for (int i = 0; i != N; ++i)
       std::construct_at(ptr_ + i, 42);
   }
@@ -35,11 +36,11 @@ struct ElementPool {
   constexpr ~ElementPool() {
     for (int i = 0; i != N; ++i)
       std::destroy_at(ptr_ + i);
-    std::allocator<T>().deallocate(ptr_, N);
+    std::allocator<std::remove_const_t<T>>().deallocate(ptr_, N);
   }
 
 private:
-  T* ptr_;
+  std::remove_const_t<T>* ptr_;
 };
 
 #endif // TEST_STD_CONTAINERS_VIEWS_MDSPAN_MINIMAL_ELEMENT_TYPE_H

Copy link
Contributor

@philnik777 philnik777 left a comment

Choose a reason for hiding this comment

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

LGTM assuming the CI is green.

@StephanTLavavej
Copy link
Member Author

CI is green, modulo unrelated timeouts and MemorySanitizer failures, I'll go ahead and merge this. Thanks!

@StephanTLavavej StephanTLavavej merged commit 7cbf959 into llvm:main Nov 28, 2023
@StephanTLavavej StephanTLavavej deleted the stl-8-allocator-const-t branch November 28, 2023 01:19
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.

3 participants