[libcxx-commits] [PATCH] D112976: [libc++][NFC] Inline most of `__vector_base` into `vector`.

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

Quuxplusone added a comment.

LGTM mod kinda-pre-existing comments.



================
Comment at: libcxx/include/vector:845-847
+      _NOEXCEPT_(
+          !__alloc_traits::propagate_on_container_move_assignment::value ||
+          is_nothrow_move_assignable<allocator_type>::value)
----------------
FWIW, this manual `noexcept`-specification scares me. Nobody actually checks whether this function is `noexcept`, do they? If so, then all we're doing via this specification is asking the compiler to insert a call to `std::terminate` in the more-or-less-likely case that we got the condition wrong. I'd rather just remove lines 845–847.
(Either this removal will make an existing regression test fail; or else it's totally a good idea; or else we need to add a regression test for it. I believe #2 is the right hypothesis.)


================
Comment at: libcxx/include/vector:885-895
+  _LIBCPP_INLINE_VISIBILITY
+  void __move_assign_alloc(vector& __c, true_type)
+      _NOEXCEPT_(is_nothrow_move_assignable<allocator_type>::value)
+  {
+    __alloc() = _VSTD::move(__c.__alloc());
+  }
+
----------------
I'd prefer to see these tag-dispatch helpers moved up directly under the single-argument `__move_assign_alloc` that uses them. Ditto for `__copy_assign_alloc`. Bonus points if we can find a better function name for the helper, e.g. `__move_assign_alloc_impl`, so that we don't have one- and two-argument overloads of the same name.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112976