[libcxx-commits] [PATCH] D132061: [libc++] static_assert preconditions in vector

[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/allocator_traits.h:369
 struct __is_default_allocator<allocator<_Tp> > : true_type { };
 
+template <class _Alloc, class... _Args>
----------------
Let's add a comment like:

// Implement named requirements from http://eel.is/c++draft/container.alloc.reqmts

And then, while we're at it, we should also implement `Cpp17EmplaceConstructible` and other named requirements, and probably check them in the places where they are mandated.


================
Comment at: libcxx/include/__memory/allocator_traits.h:370
 
-// __is_cpp17_move_insertable
-template <class _Alloc, class = void>
-struct __is_cpp17_move_insertable
-    : is_move_constructible<typename _Alloc::value_type>
-{ };
+template <class _Alloc, class... _Args>
+decltype(allocator_traits<_Alloc>::construct(std::declval<_Alloc&>(),
----------------
Observation: our previous definition was wrong, since it looked for a `Allocator.construct()` method only, and the `Cpp17FooInsertable` requirements are based on `allocator_traits<Alloc>::construct`. Your implementation does it the right way. Can you please add a test that would fail if we did not do it correctly? This should be possible by creating an allocator `A` without a `.construct()` method, but defining `allocator_traits<A>::construct` (or, in C++<something>, `allocator_traits::construct` might even be defined for you automatically, although the test should work in older standards too so maybe don't rely on that).



================
Comment at: libcxx/include/__memory/allocator_traits.h:403
 
 _LIBCPP_POP_MACROS
 
----------------
For LLVM 15, I would be tempted to go and remove the static assert in `__uninitialized_allocator_move_if_noexcept` since it is wrong (due to checking `Allocator.construct()` instead of `allocator_traits::construct`:

```
static_assert(__is_cpp17_move_insertable<_Alloc>::value,
                "The specified type does not meet the requirements of Cpp17MoveInsertable");
```

We can also fix the trait, however I don't think this is a major issue and I would not spend a lot of time trying to backport a complicated fix. Can you ping me when you have a patch on top of LLVM 15? I can cherry-pick it.



================
Comment at: libcxx/test/libcxx/containers/sequences/vector/preconditions.verify.cpp:9
+
+// ensure that preconditions are checked
+
----------------
This is kind of light on the details.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132061