[libcxx-commits] [PATCH] D132149: [String] Allow fancy pointer as pointer type of basic_string allocator

Nikolas Klauser via Phabricator via libcxx-commits libcxx-commits at lists.llvm.org
Tue Dec 6 01:36:27 PST 2022


philnik added inline comments.


================
Comment at: libcxx/include/string:778
 
+#ifdef _LIBCPP_CXX03_LANG
     union __ulx{__long __lx; __short __lxx;};
----------------
Why is this special-casing needed? AFAICT the code below is valid in C++03.


================
Comment at: libcxx/include/string:691
                   "Allocator::value_type must be same type as value_type");
+    static_assert((is_trivially_destructible<pointer>::value), "pointer type must have a trivial destructor.");
 
----------------
bemeryesr wrote:
> philnik wrote:
> > AFAICT pointers don't have to have trivial destructors.
> Yeah, you're right. As of C++20, we can use constexpr destructors. Before C++20 they must be trivial in order for basic_string to be constexpr.
`basic_string` isn't `constexpr` before C++20. Either way, we have to support non-`constexpr` pointers. AFAIK nothing in the standard requires the pointer to work during constant evaluation.


================
Comment at: libcxx/test/libcxx/strings/basic.string/string.access/assert.back.pass.cpp:22
 #include "min_allocator.h"
+#include "test_allocator.h"
+
----------------
Please move these test refactorings into it's own PR. This diff is really large.


================
Comment at: libcxx/test/libcxx/strings/basic.string/string.capacity/max_size.pass.cpp:94
+
+#  if TEST_STD_VER >= 11
+
----------------
It doesn't seem like this change adds any coverage. The `fancy_pointer_allocator` doesn't change anything w.r.t. `max_size`, does it?


================
Comment at: libcxx/test/libcxx/strings/basic.string/string.capacity/max_size.pass.cpp:115
 #if TEST_STD_VER > 17
   // static_assert(test());
 #endif
----------------
Could you enable the `constexpr` test as a drive-by?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D132149/new/

https://reviews.llvm.org/D132149



More information about the libcxx-commits mailing list