1129559 - MaybeOneOf should have a move constructor

Status: RESOLVED FIXED

(Core :: MFBT, defect)

Description

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

We should be able to move construct MaybeOneOf.

Comment 1

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Implement move construction for mozilla::MaybeOneOf

Comment 2

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Implement move construction for mozilla::MaybeOneOf

Comment 3

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Needed to be explicit with types and the construct<T> method or my uses wouldn't compile.

Comment 4

[Jan de Mooij [:jandem]]

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;
  }

Comment 5

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cd19700f9381

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/cd19700f9381