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

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

Quuxplusone accepted this revision as: Quuxplusone.
Quuxplusone added subscribers: Mordante, ldionne.
Quuxplusone added a comment.

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.



================
Comment at: libcxx/test/std/containers/associative/set/set.cons/assign_initializer_list.pass.cpp:63
+    LIBCPP_ASSERT(alloc_stats.alloc_count == 0);
+    Set s({1, 2, 3}, std::less<int>(), test_allocator<int>(&alloc_stats));
+    LIBCPP_ASSERT(alloc_stats.alloc_count == 3);
----------------
This change is mildly gross, but I think I agree that it has no ill effect on what this test is trying to test.


================
Comment at: libcxx/test/std/strings/basic.string/string.cons/move_alloc.pass.cpp:22
 
+test_allocator_statistics alloc_stats;
 
----------------
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.


================
Comment at: libcxx/test/support/test_allocator.h:54-55
 
-int test_alloc_base::count = 0;
-int test_alloc_base::time_to_throw = 0;
-int test_alloc_base::alloc_count = 0;
-int test_alloc_base::throw_after = INT_MAX;
-int test_alloc_base::copied = 0;
-int test_alloc_base::moved = 0;
-int test_alloc_base::converted = 0;
+TEST_CONSTEXPR const int destructed_value = -1;
+TEST_CONSTEXPR const int moved_value = INT_MAX;
 
----------------
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;
};
```


================
Comment at: libcxx/test/support/test_allocator.h:106-108
+  TEST_CONSTEXPR_CXX14 test_allocator(const test_allocator& a) TEST_NOEXCEPT : data_(a.data_),
+                                                                               id_(a.id_),
+                                                                               stats_(a.stats_) {
----------------
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. ;)


================
Comment at: libcxx/test/support/test_allocator.h:333-334
 
 template <class T>
-class other_allocator
-{
-    int data_;
+class other_allocator {
+  int data_;
----------------
>From here down, you didn't change anything except whitespace, right?
Can you revert all the diffs from here down?


================
Comment at: libcxx/test/support/test_allocator.h:500
+  limited_allocator& operator=(const limited_allocator&); // = delete;
 
 public:
----------------
philnik wrote:
> Is there any reason the copy ctor isn't really marked ##= delete##?
This is now D111148.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110994