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

[Brendan Emery via Phabricator via libcxx-commits]

bemeryesr marked 3 inline comments as done.
bemeryesr added inline comments.


================
Comment at: libcxx/include/string:778
 
+#ifdef _LIBCPP_CXX03_LANG
     union __ulx{__long __lx; __short __lxx;};
----------------
philnik wrote:
> Why is this special-casing needed? AFAICT the code below is valid in C++03.
This is needed because the C++03 standard says "An object of a class with a non-trivial default constructor (12.1), a non-trivial copy constructor (12.8), a non-trivial destructor (12.4), or a non-trivial copy assignment operator (13.5.3, 12.8) cannot be a member of a union". Since __long contains a fancy pointer which is not trivially constructible, __long itself is not trivially constructible and therefore __rep cannot include it as a member 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.");
 
----------------
philnik wrote:
> 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.
Done (Removed static assert) .


================
Comment at: libcxx/test/libcxx/strings/basic.string/string.access/assert.back.pass.cpp:22
 #include "min_allocator.h"
+#include "test_allocator.h"
+
----------------
philnik wrote:
> Please move these test refactorings into it's own PR. This diff is really large.
Done, I will add a link to the PR when I upload it later today.


================
Comment at: libcxx/test/libcxx/strings/basic.string/string.capacity/max_size.pass.cpp:94
+
+#  if TEST_STD_VER >= 11
+
----------------
philnik wrote:
> 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?
Yep, you're right. I will remove it in the separate PR.


================
Comment at: libcxx/test/libcxx/strings/basic.string/string.capacity/max_size.pass.cpp:115
 #if TEST_STD_VER > 17
   // static_assert(test());
 #endif
----------------
philnik wrote:
> Could you enable the `constexpr` test as a drive-by?
Actually, it's since been enabled (https://reviews.llvm.org/D128578#change-59T6waF87eYp). 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132149