Now that we don't have to worry about MSVC2013: https://groups.google.com/forum/#!topic/mozilla.dev.platform/-CYfd88R_Dg we can rely on implicit move member generation which we couldn't before: https://developer.mozilla.org/en-US/docs/Using_CXX_in_Mozilla_code#Notes which, iiuc, was only because of a bug in MSVC2013.

Comment on attachment 8758729 [details] [diff] [review] implicit-move Review of attachment 8758729 [details] [diff] [review]: ----------------------------------------------------------------- Good riddance, thanks! Not too sure what was the outcome of the Aurora discussion; I think on Aurora we reverted to using MSVC 2013, but not sure how long it will last. I guess we could still back this patch out on Aurora, if need be. Also, can you remove these move ctor: https://dxr.mozilla.org/mozilla-central/source/js/src/jit/x86-shared/MacroAssembler-x86-shared.h#52-53

If we've disabled MSVC2013 on trunk then surely that disablement would ride the trains b/c otherwise we'd have *tons* of bustage (like we're introducing right now :) on aurora.

Rather than remove the definitions entirely, could you just default them, e.g. |AstSig& operator=(AstSig&& rhs) = default;|, so that the intent of movability is explicit rather than spread out across a bunch of harder-to-see call sites?

I'd rather not do that haphazardly given that we don't have a general stylistic convention (or, more importantly, static check) requiring default declarations for *all* copy/move ctor/assign. I could see the value if we wanted to effectively flip the default to no-copy/no-move, but given that the default is copyable/movable, it doesn't seem particularly valuable.

(In reply to Luke Wagner [:luke] from comment #6) > I'd rather not do that haphazardly given that we don't have a general > stylistic convention (or, more importantly, static check) requiring default > declarations for *all* copy/move ctor/assign. I could see the value if we > wanted to effectively flip the default to no-copy/no-move, but given that > the default is copyable/movable, it doesn't seem particularly valuable. I don't think this is a good rationale for removing useful information.

Is it useful? It says it may be moved, but the absence doesn't say it won't be moved. If I want to add some field that isn't move-safe, I had better delete move ops, regardless. I think it's just noise without some regular convention.

(In reply to Luke Wagner [:luke] from comment #8) > Is it useful? It says it may be moved, but the absence doesn't say it won't > be moved. Its absence leaves it unclear whether it can be moved. I mean, sure, you could look at members, declared constructors and assignment operators, and superclasses to determine that, but wouldn't you rather look at one line of code? > I think it's just noise without some regular convention. *shrug* OK, we can disagree here. Can you file a static analysis bug for requiring explicit, |= default|, or |= delete| for appropriate member functions?

(In reply to Nathan Froyd [:froydnj] from comment #9) > Its absence leaves it unclear whether it can be moved. Unless there is more recent thinking on this (I'm not as up-to-date on this as I used to be), the C++ convention is that, if you don't delete/hide one of these implicit ops (and they aren't suppressed) you should be able to do it. According to this convention, the burden is on the author to *disable* copy/move when it's not safe/intended. Now I can appreciate wanting to change the convention within Mozilla (globally, checked by tools) to force everyone to ask this question (like we did with MOZ_IMPLICIT), but that's a separate and broader discussion. > *shrug* OK, we can disagree here. Can you file a static analysis bug for > requiring explicit, |= default|, or |= delete| for appropriate member > functions? To be clear I think it's totally worth having the broader discussion but since there is no such convention now, I don't see this patch as doing anything more than removing member definitions that I wouldn't have written in the first place if they didn't fail to compile on MSVC2013. Seeing as I'm not actually interested in advocating to change the status quo here, could you file the static analysis bug instead?

Pushed by lwagner@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/067521ccd179 remove some move member definitions that can be implicitly generated (r=bbouvier)