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

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik added inline comments.


================
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&>(),
----------------
ldionne wrote:
> 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).
> 
AFAICT `construct` was always optional, so I think it's OK to just use an allocator without `construct`.


================
Comment at: libcxx/test/libcxx/containers/sequences/vector/preconditions.verify.cpp:9
+
+// ensure that preconditions are checked
+
----------------
ldionne wrote:
> This is kind of light on the details.
A single section doesn't apply here, and I don't think it makes sense to list the subsections of `[vector]`. Saying it's in `[vector]` also seems quite obvious.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132061