This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by philnik on Oct 2 2021, 6:02 AM.

Details

Summary

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

Diff Detail

Event Timeline

philnik requested review of this revision.Oct 2 2021, 6:02 AM
philnik created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptOct 2 2021, 6:02 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Quuxplusone requested changes to this revision.Oct 2 2021, 5:20 PM

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."

This revision now requires changes to proceed.Oct 2 2021, 5:20 PM
philnik updated this revision to Diff 377187.Oct 5 2021, 6:28 AM

Defaulting now to nullptr

philnik marked 2 inline comments as done.Oct 5 2021, 6:49 AM

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
502

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

philnik updated this revision to Diff 377228.Oct 5 2021, 7:06 AM

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

philnik updated this revision to Diff 377229.Oct 5 2021, 7:08 AM

Didn't upload all stuff

jloser added a subscriber: jloser.Oct 5 2021, 9:41 AM
jloser added inline comments.
libcxx/test/support/test_allocator.h
502

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

Quuxplusone added subscribers: Mordante, ldionne.

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

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"?
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?

502

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
101

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

108

Nit: Please make TEST_NOEXCEPT.

philnik updated this revision to Diff 377354.Oct 5 2021, 2:09 PM
philnik marked 8 inline comments as done.
  • 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 marked an inline comment as done and an inline comment as not done.Oct 5 2021, 2:14 PM
philnik added inline comments.
libcxx/test/std/strings/basic.string/string.cons/move_alloc.pass.cpp
22

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 updated this revision to Diff 379031.Oct 12 2021, 7:33 AM
  • added test_allocator_statistics to allocate_shared.pass.cpp

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–254

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 updated this revision to Diff 380188.Oct 16 2021, 8:23 AM
  • Made the rest of test_allocator.h constexpr
philnik added inline comments.Oct 19 2021, 3:40 PM
libcxx/test/support/test_allocator.h
214–254

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 requested changes to this revision.Oct 20 2021, 2:26 PM

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
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–254

Please perform that removal in a separate patch.

This revision now requires changes to proceed.Oct 20 2021, 2:26 PM
philnik updated this revision to Diff 381254.Oct 21 2021, 7:15 AM

Factored out formatting to D112219

philnik updated this revision to Diff 381255.Oct 21 2021, 7:18 AM

Again didn't upload all

philnik updated this revision to Diff 381256.Oct 21 2021, 7:22 AM

Next try...

philnik updated this revision to Diff 381257.Oct 21 2021, 7:26 AM

Ok, now it should be correct

ldionne requested changes to this revision.Oct 28 2021, 1:39 PM
ldionne added inline comments.
libcxx/test/std/containers/associative/map/map.cons/assign_initializer_list.pass.cpp
24

Gentle ping on this comment.

This revision now requires changes to proceed.Oct 28 2021, 1:39 PM
philnik updated this revision to Diff 383307.Oct 29 2021, 4:45 AM
philnik marked an inline comment as done.
  • Made test_allocator_statistics local
philnik marked 3 inline comments as done.Oct 29 2021, 4:47 AM
ldionne accepted this revision.Oct 29 2021, 8:31 AM
This revision is now accepted and ready to land.Oct 29 2021, 8:31 AM

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

philnik updated this revision to Diff 383568.Oct 30 2021, 3:51 AM
  • moved test_allocator_statistics

Someone needs to commit this patch for me. "Nikolas Klauser" <nikolasklauser@berlin.de>

This revision was landed with ongoing or failed builds.Nov 7 2021, 7:16 AM
This revision was automatically updated to reflect the committed changes.
Via Conduit