[libcxx-commits] [PATCH] D110994: [libc++] Make test_allocator constexpr-friendly for constexpr string/vector

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone requested changes to this revision.
Quuxplusone added a comment.
This revision now requires changes to proceed.

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. :)



================
Comment at: libcxx/test/support/test_allocator.h:69
+    test_allocator_statistics* stats_;
+};
 
----------------
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.


================
Comment at: libcxx/test/support/test_allocator.h:90
 
-    test_allocator() TEST_NOEXCEPT : data_(0), id_(0) {++count;}
-    explicit test_allocator(int i, int id = 0) TEST_NOEXCEPT : data_(i), id_(id)
-      {++count;}
-    test_allocator(const test_allocator& a) TEST_NOEXCEPT : data_(a.data_),
+    TEST_CONSTEXPR_CXX14 test_allocator(test_allocator_statistics* stats = &alloc_stats) TEST_NOEXCEPT
+        : test_alloc_base(stats), data_(0), id_(0)
----------------
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."


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110994