Make test_allocator etc. constexpr-friendly so they can be used to test constexpr string and possibly constexpr vector
Details
- Reviewers
• Quuxplusone ldionne - Group Reviewers
Restricted Project - Commits
- rG9a140a1586cc: [libc++] Make test_allocator constexpr-friendly for constexpr string/vector
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
I don't believe this gets us all the way to constexpr-friendliness yet; see my comments and please let me know if anything I said fails to make sense. :)
libcxx/test/support/test_allocator.h | ||
---|---|---|
67 | You've correctly intuited about half of the fix I intended. :) So this is a step in the right direction, but you need to take one more step before it'll fix the constexprness problem. test_allocator_statistics alloc_stats; // LOCAL VARIABLE { typedef test_allocator<char> A; typedef std::basic_string<char, std::char_traits<char>, A> S; #if TEST_STD_VER > 14 static_assert((noexcept(S{})), "" ); #elif TEST_STD_VER >= 11 static_assert((noexcept(S()) == std::is_nothrow_move_constructible<A>::value), ""); #endif S s1("Twas brillig, and the slivy toves did gyre and gymbal in the wabe"); S s2(std::move(s1), A(1, &alloc_stats)); // DIFFERENCE HERE } assert(alloc_stats.alloc_count == alloc_count); And then this test will be trivially constexpr-ible, because it won't depend on any global runtime state. I'm assuming that as part of this transformation, the test_alloc_base class will actually go away, because now it doesn't really serve any purpose. But maybe I'm missing something. | |
67–68 | This ctor overload set will become more like test_allocator_statistics *stats_ = nullptr; int data_ = 0; // participates in equality int id_ = 0; // unique identifier, doesn't participate in equality ~~~ explicit test_allocator() = default; TEST_CONSTEXPR explicit test_allocator(int data) TEST_NOEXCEPT : data_(data) {} TEST_CONSTEXPR explicit test_allocator(int data, int id) TEST_NOEXCEPT : data_(data), id_(id) {} TEST_CONSTEXPR_CXX14 explicit test_allocator(int data, test_allocator_statistics *stats) TEST_NOEXCEPT : stats_(stats), data_(data) { stats_->count += 1; } TEST_CONSTEXPR_CXX14 test_allocator(const test_allocator& a) TEST_NOEXCEPT : stats_(a.stats), data_(a.data), id_(a.id_) { if (stats_ != nullptr) { stats_->count += 1; stats_->copied += 1; } assert(a.data_ != destructed_value && a.id_ != destructed_value && "copying from destroyed allocator"); } and so on. (Notice that it's OK to use NSDMIs and =default in C++03 mode, because Clang supports them as extensions, and we don't claim to support any non-Clang compilers prior to C++11 mode.) |
I know you don't like clang-format, but (to me) the formatting in test_allocator.h felt like a complete clusterfuck. Maybe it's just me, but I edited large parts of the file anyways.
libcxx/test/support/test_allocator.h | ||
---|---|---|
523 | Is there any reason the copy ctor isn't really marked = delete? |
Replaced assert(data_ >= 0) with assert(data_ != destructed_value)
Made the special values constexpr and removed default_value because it was unused
libcxx/test/support/test_allocator.h | ||
---|---|---|
523 | FYI I changed this in https://reviews.llvm.org/D111148 |
At this point I'm happy, modulo some last formatting/naming comments. Of course assuming that (1) buildkite is happy, and (2) you've tested rebasing your constexpr string PR on top of this and confirmed that it does successfully do all you need in order to make those string-allocator tests constexpr-friendly.
Leaving it to @ldionne or @Mordante to give libc++ approval.
libcxx/test/std/containers/associative/set/set.cons/assign_initializer_list.pass.cpp | ||
---|---|---|
62 | This change is mildly gross, but I think I agree that it has no ill effect on what this test is trying to test. | |
libcxx/test/std/strings/basic.string/string.cons/move_alloc.pass.cpp | ||
22 | For this string test in particular: Are you confident that the step from this point, to TEST_CONSTEXPR_CXX20, is a small step that "just works"?
But you should try it locally and make sure we haven't missed anything. | |
libcxx/test/support/test_allocator.h | ||
54–58 | I think having a global variable named moved_value makes it kind of confusing at the call-site — looks too much like it might be a local variable or something. Doesn't look "global enough." struct test_alloc_base { TEST_CONSTEXPR const int destructed_value = -1; TEST_CONSTEXPR const int moved_value = INT_MAX; }; | |
86–88 | And likewise throughout. There's nothing wrong with using clang-format as the starting point for your new code, but let's not just replace one clusterfrak with another. ;) | |
325–327 | From here down, you didn't change anything except whitespace, right? | |
523 | This is now D111148. |
A few small nits, but I want to have a closer look when I have more time, unless Louis beats me to it.
Can you make sure you mark all comments done as done?
libcxx/test/support/test_allocator.h | ||
---|---|---|
69 | Nit: Only a part of the constructors is explicit; please make all appropriate ones explicit. | |
76 | Nit: Please make TEST_NOEXCEPT. |
- Moved destructed_value and moved_value back in test_alloc_base and made other_allocator constexpr(needed for constexpr string)
- Merge branch 'main' of https://github.com/llvm/llvm-project
libcxx/test/support/test_allocator.h | ||
---|---|---|
247–255 | Based on what I did in D68365, I think the correct definition of this would be: #if TEST_STD_VER < 11 void construct(pointer p, const T& val) { ::new(static_cast<void*>(p)) T(val); } #elif TEST_STD_VER < 20 template <class U> void construct(pointer p, U&& val) { ::new(static_cast<void*>(p)) T(std::forward<U>(val)); } #else template <class U> constexpr void construct(pointer p, U&& val) { std::construct_at(p, std::forward<U>(val)); } #endif |
libcxx/test/support/test_allocator.h | ||
---|---|---|
247–255 | I think it can just be removed, since it doesn't ever get used anyways and is removed in C++20 from std::allocator. |
Roughly LGTM, but I'd really like to avoid making orthogonal changes in this patch. As it stands, this is basically a complete rewrite of test_allocator.h, not just "making the test allocators constexpr friendly".
In libc++, we generally try to make orthogonal changes in separate patches because it makes it easier to validate correctness. I'm sorry for requesting that - I know it means more work for something that you might not see value in since you wrote the patch, but for reviewers it really helps.
libcxx/test/std/containers/associative/map/map.cons/assign_initializer_list.pass.cpp | ||
---|---|---|
24 | While we're at it, can we avoid making these allocation stats global? We could define the stats as local variables in the functions. | |
libcxx/test/support/test_allocator.h | ||
60 | Please avoid making wide formatting changes in this patch. I much prefer the way you have it right now, but mixing it with the patch to constexpr-ify these classes makes it a lot harder to validate this patch. Please split off the formatting in a separate patch -- I don't care if it comes before or after. | |
247–255 | Please perform that removal in a separate patch. |
libcxx/test/std/containers/associative/map/map.cons/assign_initializer_list.pass.cpp | ||
---|---|---|
24 | Gentle ping on this comment. |
Someone needs to commit this patch for me. "Nikolas Klauser" <nikolasklauser@berlin.de>
While we're at it, can we avoid making these allocation stats global? We could define the stats as local variables in the functions.