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

[Christopher Di Bella via Phabricator via libcxx-commits]

cjdb added inline comments.


================
Comment at: libcxx/test/std/concepts/object/movable.compile.pass.cpp:12
+
+// template<class From, class To>
+// concept movable = see below;
----------------
Mordante wrote:
> Please remove `class From, `.
Fixed properly this time.


================
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>);
+
----------------
Quuxplusone wrote:
> 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).
I //think// we're in agreement that these can stay per https://reviews.llvm.org/D97443#inline-920565.


================
Comment at: libcxx/test/std/concepts/object/movable.h:49
+struct const_move_constructor {
+  const_move_constructor& operator=(const_move_constructor&&) const;
+};
----------------
Quuxplusone wrote:
> 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&)`?
> This is not a move constructor.

Wow.

> 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&)?

I only expected this overload, since it's a user-declared move assignment operator. I've added a few new tests around this topic.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97359