Closed Bug 1129559 Opened 5 years ago Closed 5 years ago

MaybeOneOf should have a move constructor

Categories

(Core :: MFBT, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox38 --- fixed

People

(Reporter: fitzgen, Assigned: fitzgen)

References

Details

Attachments

(1 file, 1 obsolete file)

We should be able to move construct MaybeOneOf.
Attachment #8559300 - Attachment is obsolete: true
Attachment #8559300 - Flags: review?(jdemooij)
Attachment #8559341 - Flags: review?(jdemooij)
Needed to be explicit with types and the construct<T> method or my uses wouldn't compile.
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+
https://hg.mozilla.org/mozilla-central/rev/cd19700f9381
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
You need to log in before you can comment on or make changes to this bug.