Closed
Bug 1277247
Opened 8 years ago
Closed 8 years ago
remove move member definitions that can be implicitly generated
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla50
People
(Reporter: luke, Assigned: luke)
References
Details
Attachments
(1 file)
7.24 KB,
patch
|
bbouvier
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•8 years ago
|
||
Attachment #8758729 -
Flags: review?(bbouvier)
Assignee | ||
Comment 2•8 years ago
|
||
Comment 3•8 years ago
|
||
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
Attachment #8758729 -
Flags: review?(bbouvier) → review+
Assignee | ||
Comment 4•8 years ago
|
||
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.
Comment 5•8 years ago
|
||
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?
Assignee | ||
Comment 6•8 years ago
|
||
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.
Comment 7•8 years ago
|
||
(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.
Assignee | ||
Comment 8•8 years ago
|
||
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.
Comment 9•8 years ago
|
||
(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?
Assignee | ||
Comment 10•8 years ago
|
||
(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?
Comment 11•8 years ago
|
||
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)
Comment 12•8 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox50:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in
before you can comment on or make changes to this bug.
Description
•