[libcxx-commits] [PATCH] D69132: [libc++][P0784] Marked the default allocator constexpr.

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added inline comments.
This revision now requires changes to proceed.


================
Comment at: libcxx/include/memory:1957
+    _LIBCPP_DEPRECATED_IN_CXX17 _LIBCPP_INLINE_VISIBILITY
+    _LIBCPP_CONSTEXPR_AFTER_CXX17_WITH_CONSTEXPR_DYNAMIC_ALLOC
+    void destroy(pointer __p) {__p->~_Tp();}
----------------
This is because `__cpp_constexpr_dynamic_alloc` also controls whether you can call a destructor explicitly inside `constexpr`, right? It looks kind of funny, because we don't allocate here.

**EDIT**: I guess the same hold for `construct`, where we use placement-new.


================
Comment at: libcxx/include/new:261
+  template <class _A1, class _A2>
+  static inline _LIBCPP_CONSTEXPR_AFTER_CXX17_WITH_CONSTEXPR_DYNAMIC_ALLOC
+  void __do_call(void *__ptr, _A1 __a1, _A2 __a2) {
----------------
I assume you had to move the `__do_call` functions here because making them `constexpr` somehow required their definition to appear above the definition of the various `__do_allocate` functions below?


================
Comment at: libcxx/test/std/utilities/memory/default.allocator/allocator.members/allocate.fail.cpp:29
+    static_assert([]() constexpr {  // expected-error {{static_assert expression is not an integral constant expression}}
+        std::allocator<int> a;
+        TEST_IGNORE_NODISCARD a.allocate(3);
----------------
I don't understand that test. Shouldn't allocator be made constexpr by this patch? Why do you check that there's an error saying it's not an ICE?


================
Comment at: libcxx/test/std/utilities/memory/default.allocator/allocator.members/allocate.pass.cpp:83
+constexpr bool test_aligned_constexpr() {
+    typedef AlignedType<Align> T;
+    std::allocator<T> a;
----------------
You have a mismatch in the indentation of the return and body.


================
Comment at: libcxx/test/std/utilities/memory/default.allocator/allocator.members/allocate.size.fail.cpp:38
+{
+    static_assert(test<double>());  // expected-error {{static_assert expression is not an integral constant expression}}
+    LIBCPP_STATIC_ASSERT(test<const double>());  // expected-error {{static_assert expression is not an integral constant expression}}
----------------
I would expect that you get one error for each statement up in `test()` above. Isn't it what's happening? I would expect something like `expected-error 4 {{...}}`.


================
Comment at: libcxx/test/std/utilities/memory/default.allocator/allocator.members/allocate.size.fail.cpp:39
+    static_assert(test<double>());  // expected-error {{static_assert expression is not an integral constant expression}}
+    LIBCPP_STATIC_ASSERT(test<const double>());  // expected-error {{static_assert expression is not an integral constant expression}}
+
----------------
Why not just `static_assert` here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69132