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

[Louis Dionne via Phabricator via libcxx-commits]

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

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.



================
Comment at: libcxx/test/std/containers/associative/map/map.cons/assign_initializer_list.pass.cpp:24
 
+test_allocator_statistics alloc_stats;
+
----------------
While we're at it, can we avoid making these allocation stats global? We could define the stats as local variables in the functions.


================
Comment at: libcxx/test/support/test_allocator.h:69
 public:
+  typedef unsigned size_type;
+  typedef int difference_type;
----------------
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.


================
Comment at: libcxx/test/support/test_allocator.h:256-263
 #if TEST_STD_VER < 11
-    void construct(pointer p, const T& val)
-        {::new(static_cast<void*>(p)) T(val);}
+  void construct(pointer p, const T& val) { ::new (static_cast<void*>(p)) T(val); }
 #else
-    template <class U> void construct(pointer p, U&& val)
-        {::new(static_cast<void*>(p)) T(std::forward<U>(val));}
+  template <class U>
+  void construct(pointer p, U&& val) {
+    ::new (static_cast<void*>(p)) T(std::forward<U>(val));
+  }
----------------
philnik wrote:
> ldionne wrote:
> > 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
> > ```
> > 
> I think it can just be removed, since it doesn't ever get used anyways and is removed in C++20 from ##std::allocator##.
Please perform that removal in a separate patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110994