Closed
Bug 1129559
Opened 6 years ago
Closed 6 years ago
MaybeOneOf should have a move constructor
Categories
(Core :: MFBT, defect)
Tracking
()
RESOLVED
FIXED
mozilla38
Tracking | Status | |
---|---|---|
firefox38 | --- | fixed |
People
(Reporter: fitzgen, Assigned: fitzgen)
References
Details
Attachments
(1 file, 1 obsolete file)
1.18 KB,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
We should be able to move construct MaybeOneOf.
Assignee | ||
Comment 1•6 years ago
|
||
Attachment #8559300 -
Flags: review?(jdemooij)
Assignee | ||
Comment 2•6 years ago
|
||
Attachment #8559300 -
Attachment is obsolete: true
Attachment #8559300 -
Flags: review?(jdemooij)
Attachment #8559341 -
Flags: review?(jdemooij)
Assignee | ||
Comment 3•6 years ago
|
||
Needed to be explicit with types and the construct<T> method or my uses wouldn't compile.
Comment 4•6 years ago
|
||
Comment on attachment 8559341 [details] [diff] [review] Implement move construction for mozilla::MaybeOneOf Review of attachment 8559341 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me with comments below addressed. Nice to see MaybeOneOf being used elsewhere. ::: mfbt/MaybeOneOf.h @@ +50,5 @@ > public: > MaybeOneOf() : state(None) {} > ~MaybeOneOf() { destroyIfConstructed(); } > > + MaybeOneOf(MaybeOneOf &&rhs) Nit: MFBT uses Gecko style (although this file doesn't use it consistently yet) and it's probably nice to match Maybe, so: MaybeOneOf(MaybeOneOf&& aOther) @@ +65,5 @@ > + rhs.state = None; > + } > + } > + > + MaybeOneOf &operator=(MaybeOneOf &&rhs) Here I'd also match Maybe and assert against self-assignment: MaybeOneOf& operator=(MaybeOneOf&& aOther) { MOZ_ASSERT(this != &aOther, "Self-moves are prohibited"); this->~MaybeOneOf(); new(this) MaybeOneOf(Move(aOther)); return *this; }
Attachment #8559341 -
Flags: review?(jdemooij) → review+
Assignee | ||
Comment 5•6 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/cd19700f9381
Comment 6•6 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/cd19700f9381
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox38:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
You need to log in
before you can comment on or make changes to this bug.
Description
•