[libcxx-commits] [PATCH] D97359: [libcxx] adds concept std::movable

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

Quuxplusone accepted this revision as: Quuxplusone.
Quuxplusone added a comment.

As usual, the code in <concepts> is "obviously correct" (and the tests are more verbose than I'd wish, but I don't intend to hold it up over that). LGTM%nits, if tests pass.



================
Comment at: libcxx/test/std/concepts/object/movable.compile.pass.cpp:113
+static_assert(!std::movable<has_rvalue_reference_member>);
+static_assert(!std::movable<has_function_ref_member>);
+
----------------
I don't see what these cases have to do with the concept; aren't these just a list of ways a type can fail to be assignable at all? I'd just write one non-assignable type right here in this file and then test that //one//.
But I guess since you've already written all the code, maybe it's too late to ask for it to be shortened (or maybe you only lengthened it in the first place due to someone else's feedback, I dunno).


================
Comment at: libcxx/test/std/concepts/object/movable.h:49
+struct const_move_constructor {
+  const_move_constructor& operator=(const_move_constructor&&) const;
+};
----------------
This is not a move constructor.
Also, I don't know what the overload set ends up looking like here; do you expect there to be a defaulted `operator=(const_move_constructor&&)` as well? What about a defaulted `operator=(const const_move_constructor&)`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97359