Right now Maybe<T> supports all copy and move constructors and operators all the time, and relies on them failing to compile when used. The problem is that Visual Studio 2010 instantiates those constructors and operators even if they aren't used. This has prevented a correct implementation of the copy assignment operator, and will potentially cause more problems down the road. A correct fix requires implementing equivalents of std::is_copy_assignable and std::is_move_assignable, moving most of the functionality of Maybe<T> into a base class, and then creating different Maybe<T> specializations that expose the different constructors and operators according to the capabilities of the underlying type T. We should also be sure to fix the copy assignment operator and the corresponding test in TestMaybe.cpp. (Search Maybe.h and TestMaybe.cpp for this bug number to find the locations that need fixing.)

Since we have dropped VS 2010 support, are you going to fix this bug now?

Yes, but not in the next week or so. I've got some other more important bugs that need fixing.

The test case is pretty interesting... It seems implementing Is{Copy,Move}Assignable is not enough. We also need Is{Copy,Move}Constructible for that. There is one more issue for emplace() with deleted move constructor. We probably can add an overload to trap this single case when the type is not move-constructible.

Actually it seems to me that it makes no sense to have unmovable object. An object with move constructor deleted won't be able to accept any rvalue. Thus I think we probably should remove the unmovable object from TestMaybe.

JS::Rooted variants should never move and they certainly make sense.

(In reply to Nick Fitzgerald [:fitzgen][:nf] from comment #5) > JS::Rooted variants should never move and they certainly make sense. It should never be moved, and it should never be copyed, either, right? I meant, there shouldn't exist object which is copy-constructible but not move-constructible. A object which is neither copy-constructible nor move-constructible certainly makes sense. Well... I just did a search and found some classes do remove the move constructor but keep the copy constructor.

In our codebase, I found only one class which defines copy constructor but deletes move constructor, which is js/src/gc/Barrier.h:HeapPtr. Not sure why it is defined that way...

Terrence, perhaps you can shed light on comment 7?

It seems to me the copy constructor of HeapPtr can be safely deleted as well. No one actually relies on that constructor. Theoretically, if a type is copy-constructible, it should natively be move-constructible: it can just copy things instead of doing any movement. Explicitly deleting move constructor but keeping copy constructor doesn't make much sense, since compilers won't generate move constructor if there is any copy constructor, and any constructing with rvalue would just use the copy constructor.

(In reply to Xidorn Quan [:xidorn] (UTC+12) from comment #7) > In our codebase, I found only one class which defines copy constructor but > deletes move constructor, which is js/src/gc/Barrier.h:HeapPtr. Not sure why > it is defined that way... HeapPtr encodes the address of its encapsulated pointer in a dependent external resource. Unlike RelocatablePtr, HeapPtr does not support removal of that resource, only append; thus, Copy makes sense, but Move does not. This was a conscious design decision made for performance and still nets of 2+% on octane. (In reply to Xidorn Quan [:xidorn] (UTC+12) from comment #9) > It seems to me the copy constructor of HeapPtr can be safely deleted as > well. No one actually relies on that constructor. > > Theoretically, if a type is copy-constructible, it should natively be > move-constructible: it can just copy things instead of doing any movement. That doesn't really follow, for the (admittedly insane) reasons listed above. > Explicitly deleting move constructor but keeping copy constructor doesn't > make much sense, since compilers won't generate move constructor if there is > any copy constructor, and any constructing with rvalue would just use the > copy constructor.

Pushed by sgiesecke@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/95e311eb937e Make Maybe<T> only declare copy/move operations if T is copyable/movable. r=froydnj,jgilbert