[libc++] Make test_allocator constexpr-friendly for constexpr string/vector

[philnik]

Summary

Make test_allocator etc. constexpr-friendly so they can be used to test constexpr string and possibly constexpr vector

[• Quuxplusone]

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.
The next step is to get rid of the global ::alloc_stats. Instead, each caller is going to do something like (e.g. in move_alloc.pass.cpp)

    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.

98–99

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.)
Also notice that I'm drive-by applying my general guidelines of "one signature, one declaration" — default arguments are the devil — and "mark all ctors explicit except for copy and move ctors"; and applying my specific aforementioned guideline of "use the most aggressive constexpr macros possible, in test/support code."

[philnik]

Defaulting now to nullptr

[philnik]

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

482

Is there any reason the copy ctor isn't really marked = delete?

[philnik]

Replaced assert(data_ >= 0) with assert(data_ != destructed_value)
Made the special values constexpr and removed default_value because it was unused

[philnik]

Didn't upload all stuff

[jloser]

libcxx/test/support/test_allocator.h

482

FYI I changed this in https://reviews.llvm.org/D111148

[• Quuxplusone]

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

63 ↗

(On Diff #377229)

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 ↗

(On Diff #377229)

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"?
The step should be just

• Move alloc_stats into a local variable of test()
• Mark test() as TEST_CONSTEXPR_CXX20 and have it return bool
• static_assert on it

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."
I suggest we actually keep test_alloc_base for now (but not as a base class) — not because it's a good name, but just so that the diff becomes smaller. Right now, you're touching some files only to rename test_alloc_base::moved_value into moved_value, so by keeping the class around, we can remove those files from the diff.

struct test_alloc_base {
    TEST_CONSTEXPR const int destructed_value = -1;
    TEST_CONSTEXPR const int moved_value = INT_MAX;
};

118–120

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?
Can you revert all the diffs from here down?

482

This is now D111148.

[Mordante]

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

101

Nit: Only a part of the constructors is explicit; please make all appropriate ones explicit.

108

Nit: Please make TEST_NOEXCEPT.

[philnik]

• 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

[philnik]

libcxx/test/std/strings/basic.string/string.cons/move_alloc.pass.cpp

22 ↗

(On Diff #377229)

I merged the two and it works now with the current PR.

libcxx/test/support/test_allocator.h

326–327

I know this is another PR, but maybe other_allocator should be renamed to not_noexcept_allocator or something similar?

[philnik]

rebased

[philnik]

• added test_allocator_statistics to allocate_shared.pass.cpp

[ldionne]

There is some overlap with D68365 (let's move forward with this patch and I can rebase onto it). You might want to take a look at what I did in D68365 in case that gives you ideas. I also think we'll want to pick parts of D68365 out (like the shared_ptr emulation).

libcxx/test/support/test_allocator.h

214–247

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

[philnik]

• Made the rest of test_allocator.h constexpr

[philnik]

libcxx/test/support/test_allocator.h

214–247

I think it can just be removed, since it doesn't ever get used anyways and is removed in C++20 from std::allocator.

[ldionne]

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 ↗

(On Diff #380188)

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

69

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.

214–247

Please perform that removal in a separate patch.

[philnik]

Factored out formatting to D112219

[philnik]

Again didn't upload all

[philnik]

Next try...

[philnik]

Ok, now it should be correct

[ldionne]

libcxx/test/std/containers/associative/map/map.cons/assign_initializer_list.pass.cpp

24 ↗

(On Diff #380188)

Gentle ping on this comment.

[philnik]

• Made test_allocator_statistics local

[ldionne]

Please rebase onto main, that should give you a green CI run.

[philnik]

Rebased

[philnik]

• moved test_allocator_statistics

[philnik]

Someone needs to commit this patch for me. "Nikolas Klauser" <[e-mail]>

[philnik]

Rebased

libcxx/test/support/test_allocator.h

struct test_allocator_statistics {
 
   TEST_CONSTEXPR_CXX14 void clear_ctor_counters() {
     copied = 0;
     moved = 0;
     converted = 0;
   }
 };
 
 struct test_alloc_base {
   TEST_CONSTEXPR static const int destructed_value = -1;
   TEST_CONSTEXPR static const int moved_value = INT_MAX;
 };
 
 template <class T>
 class test_allocator {
   int data_ = 0; // participates in equality
   int id_ = 0;   // unique identifier, doesn't participate in equality
   test_allocator_statistics* stats_ = nullptr;
 
   template <class U>
   friend class test_allocator;
 
 public:
   typedef unsigned size_type;
   typedef int difference_type;
   typedef T value_type;
   typedef value_type* pointer;
   typedef const value_type* const_pointer;
   typedef typename std::add_lvalue_reference<value_type>::type reference;
   typedef typename std::add_lvalue_reference<const value_type>::type const_reference;
 
   template <class U>
public:
 
   TEST_CONSTEXPR_CXX14 explicit test_allocator(int data, test_allocator_statistics* stats) TEST_NOEXCEPT
       : data_(data), stats_(stats) {
     if (stats != nullptr)
       ++stats_->count;
   }
 
   TEST_CONSTEXPR explicit test_allocator(int data, int id) TEST_NOEXCEPT : data_(data), id_(id) {}
 
   TEST_CONSTEXPR_CXX14 explicit test_allocator(int data, int id, test_allocator_statistics* stats) TEST_NOEXCEPT
       : data_(data), id_(id), stats_(stats) {
     if (stats_ != nullptr)
       ++stats_->count;
   }
 
   TEST_CONSTEXPR_CXX14 test_allocator(const test_allocator& a) TEST_NOEXCEPT
     : data_(a.data_), id_(a.id_), stats_(a.stats_) {
     assert(a.data_ != test_alloc_base::destructed_value && a.id_ != test_alloc_base::destructed_value &&
            "copying from destroyed allocator");
     if (stats_ != nullptr) {
       ++stats_->count;
       ++stats_->copied;
     }
   }
 
 #if TEST_STD_VER >= 11
   TEST_CONSTEXPR_CXX14 test_allocator(test_allocator&& a) TEST_NOEXCEPT : data_(a.data_), id_(a.id_), stats_(a.stats_) {
     if (stats_ != nullptr) {
       ++stats_->count;
       ++stats_->moved;
     }
     assert(a.data_ != test_alloc_base::destructed_value && a.id_ != test_alloc_base::destructed_value &&
            "moving from destroyed allocator");
     a.data_ = test_alloc_base::moved_value;
     a.id_ = test_alloc_base::moved_value;
   }
 #endif
 
   template <class U>
public:
 
   TEST_CONSTEXPR_CXX14
   explicit non_default_test_allocator(int i, test_allocator_statistics* stats = nullptr) TEST_NOEXCEPT
       : data_(i), stats_(stats) {
     if (stats_ != nullptr) {
       ++stats_->count;
     }
   }
 
   TEST_CONSTEXPR_CXX14
   non_default_test_allocator(const non_default_test_allocator& a) TEST_NOEXCEPT : data_(a.data_), stats_(a.stats_) {
     if (stats_ != nullptr)
       ++stats_->count;
   }
 
   template <class U>
   TEST_CONSTEXPR_CXX14 non_default_test_allocator(const non_default_test_allocator<U>& a) TEST_NOEXCEPT
       : data_(a.data_), stats_(a.stats_) {
     if (stats_ != nullptr)
       ++stats_->count;
   }
 
   TEST_CONSTEXPR_CXX20 ~non_default_test_allocator() TEST_NOEXCEPT {
     assert(data_ != test_alloc_base::destructed_value);
     if (stats_ != nullptr)
       --stats_->count;
     data_ = test_alloc_base::destructed_value;
   }
 
   TEST_CONSTEXPR pointer address(reference x) const { return &x; }
   TEST_CONSTEXPR const_pointer address(const_reference x) const { return &x; }
 
   TEST_CONSTEXPR_CXX20 pointer allocate(size_type n, const void* = nullptr) {
     assert(data_ != test_alloc_base::destructed_value);
     if (stats_ != nullptr) {
       if (stats_->time_to_throw >= stats_->throw_after)
         TEST_THROW(std::bad_alloc());
       ++stats_->time_to_throw;
       ++stats_->alloc_count;
     }
     return std::allocator<value_type>().allocate(n);
   }
 
   TEST_CONSTEXPR_CXX20 void deallocate(pointer p, size_type n) {
     assert(data_ != test_alloc_base::destructed_value);
     if (stats_ != nullptr)
       --stats_->alloc_count;
     std::allocator<value_type>().deallocate(p, n);
   }
 
public:
 
   TEST_CONSTEXPR int get_id() const { return id_; }
   TEST_CONSTEXPR int get_data() const { return data_; }
 
   TEST_CONSTEXPR friend bool operator==(const test_allocator& x, const test_allocator& y) { return x.data_ == y.data_; }
   TEST_CONSTEXPR friend bool operator!=(const test_allocator& x, const test_allocator& y) { return !(x == y); }
 };
 
 template <class T>
 class other_allocator {
   int data_ = -1;
 
   template <class U>
   friend class other_allocator;
 
 public:
   typedef T value_type;
 
   TEST_CONSTEXPR_CXX14 other_allocator() {}
public:
   template <typename U>
   constexpr TaggingAllocator(const TaggingAllocator<U>&){};
 
   template <typename... Args>
   void construct(Tag_X* p, Args&&... args) {
     ::new ((void*)p) Tag_X(Ctor_Tag{}, std::forward<Args>(args)...);
   }
 
-  template<typename U>
+  template <typename U>
   void destroy(U* p) {
     p->~U();
   }
 
   TEST_CONSTEXPR_CXX20 T* allocate(std::size_t n) { return std::allocator<T>{}.allocate(n); }
   TEST_CONSTEXPR_CXX20 void deallocate(T* p, std::size_t n) { std::allocator<T>{}.deallocate(p, n); }
 };
 #endif

 
 private:
   struct control_block {
     template <class... Args>
     TEST_CONSTEXPR control_block(Args... args) : content(std::forward<Args>(args)...) {}
     size_t ref_count = 1;
     T content;
   };
 
   control_block* block = nullptr;
 
   template <class U, class... Args>
   friend TEST_CONSTEXPR_CXX20 thread_unsafe_shared_ptr<U> make_thread_unsafe_shared(Args...);
 };
 
 template <class T, class... Args>
 TEST_CONSTEXPR_CXX20 thread_unsafe_shared_ptr<T> make_thread_unsafe_shared(Args... args) {