906912 - Add a move constructor to mozilla::LinkedList and mozilla::LinkedListElement

Status: RESOLVED FIXED

(Core :: MFBT, defect)

Description

[Justin Lebar (not reading bugmail)]

This is helpful in general and in particular for WIP patches in bug 901746.

Comment 1

[Justin Lebar (not reading bugmail)]

Attachment: Part 1, v1: Add mozilla::Move::ptr().

You can get a raw ref out of a MoveRef with operator*, but there's no easy way
to get a raw pointer out of a MoveRef.  |&*movePtr| is pretty inscrutable.

I considered calling this rawPtr() or overriding operator&.  I don't feel
strongly that this is better than the alternatives, but operator& was confusing
(operator& is not commonly overridden, so it appears to me that we're taking
the address of the MoveRef itself), and I didn't think rawPtr() was worth the
extra chars.

Comment 2

[Justin Lebar (not reading bugmail)]

Attachment: Part 2, v1: Add move constructors to mozilla::LinkedList and mozilla::LinkedListElement.

Comment 3

[(no longer active)]

It would be great if you could use C++11 move constructors instead.  The fewer usages of mozilla::Move we add, the better.

Comment 4

[Justin Lebar (not reading bugmail)]

Do you really think it's better to start using C++11's move() before we've gotten rid of mfbt/Move.h?  I'm not sure that switching to a new interface before we've deprecated the old one is a good idea; that just makes it confusing for consumers (which move() do I use?).

Comment 5

[Justin Lebar (not reading bugmail)]

> I'm not sure that switching to a new interface before we've deprecated the old one is a good 
> idea; that just makes it confusing for consumers (which move() do I use?).

Actually...if C++11 move constructors handle superclasses properly, that would be a strong reason to use them here.

Right now if I have

 class Foo : LinkedListElement<Foo>

then I can't do

  Foo::Foo(MoveRef<Foo> other) : LinkedListElement(other)

because MoveRef<Foo> doesn't cast to MoveRef<LinkedListElement<Foo> >.  I haven't figured out a clean way of working around this.

Given that essentially the only time you're going to be calling the LinkedListElement move constructor is from a subclass's move constructor, if C++11 move constructors work better here, we should probably use them.

Comment 6

[Justin Lebar (not reading bugmail)]

Comment on attachment 792466 [details] [diff] [review]
Part 1, v1: Add mozilla::Move::ptr().

Yeah, this is much better using proper C++11.

Comment 7

[Justin Lebar (not reading bugmail)]

Attachment: Patch, v2 (now with C++11 move)

Comment 8

[(no longer active)]

(In reply to comment #4)
> Do you really think it's better to start using C++11's move() before we've
> gotten rid of mfbt/Move.h?  I'm not sure that switching to a new interface
> before we've deprecated the old one is a good idea; that just makes it
> confusing for consumers (which move() do I use?).

Also, note that migrating mozilla::Move to C++11 move() is not going to be an automatic process, and cannot be done very easily unless you know the exact semantics of the code involved.

Comment 9

[Justin Lebar (not reading bugmail)]

otoh we need to land something so that there's an alternative to std::move(), right?

Comment 10

[(no longer active)]

(In reply to comment #9)
> otoh we need to land something so that there's an alternative to std::move(),
> right?

Oh yes, sorry, I thought I had mentioned that.  Good news is that it's pretty simple, see RemoveReference and mozilla::Move in attachment 779237 [details] [diff] [review].  (You should be able to steal those parts of that patch verbatim, not touching the existing mozilla::Move of course.)

Comment 11

[Justin Lebar (not reading bugmail)]

Well, this is assuming that this overload

>+inline typename RemoveReference<T>::Type&&
>+Move(T&& t)

plays well with our current definition of mozilla::Move.  If we ever call mozilla::Move() with an rvalue, this new overload will be hit, and then things won't compile.

Given that this new Move and the existing mozilla::Move are completely different idioms, I'm not sure it makes sense for them to be overloads of each other...

Comment 12

[(no longer active)]

(In reply to comment #11)
> Well, this is assuming that this overload
> 
> >+inline typename RemoveReference<T>::Type&&
> >+Move(T&& t)
> 
> plays well with our current definition of mozilla::Move.  If we ever call
> mozilla::Move() with an rvalue, this new overload will be hit, and then things
> won't compile.
> 
> Given that this new Move and the existing mozilla::Move are completely
> different idioms, I'm not sure it makes sense for them to be overloads of each
> other...

Yeah, I don't think they should be either.  I would rather call this new function something completely different (mozilla::StdMove/etc?  I'm terrible at naming things.)

Comment 13

[Jeff Walden [:Waldo]]

Rename the existing one to DeprecatedMove or something, then.  It's going away, make it ugly and obvious that no one should use it.

Comment 14

[Justin Lebar (not reading bugmail)]

Something I'm discovering as I try to use std::move instead of mozilla::Move is that a mozilla::MoveRef<T> can be passed to a constructor that takes T&, while a T&& can be cast only to |const T&|, not |T&|.

This is causing some cascading sets of changes.

Comment 15

[Justin Lebar (not reading bugmail)]

Attachment: Patch, v3

The only difference between this and v2 is that this one uses mozilla::move().

I'm in the process of renaming this to mozilla::Move() in my queue, but I figure that doesn't need to stand in our way.

Comment 16

[Jeff Walden [:Waldo]]

Comment on attachment 792880 [details] [diff] [review]
Patch, v2 (now with C++11 move)

Review of attachment 792880 [details] [diff] [review]:
-----------------------------------------------------------------

This'll want to use mozilla::Move as std::move isn't omnipresent yet, I assume.

::: mfbt/LinkedList.h
@@ +58,5 @@
>  
>  #include "mozilla/Assertions.h"
>  #include "mozilla/Attributes.h"
>  #include "mozilla/NullPtr.h"
> +#include <utility>

Blank line before this.

@@ +129,5 @@
> +      next = other.next;
> +      prev = other.prev;
> +
> +      MOZ_ASSERT(other.next->prev == &other);
> +      MOZ_ASSERT(other.prev->next == &other);

I'd move this above the next/prev assignments.  Also, before the next/prev/next->prev/prev->next assignments, I'd put a comment like so:

  /* Initialize this with |other|'s pointers, and adjust those elements to point to this one. */

@@ +132,5 @@
> +      MOZ_ASSERT(other.next->prev == &other);
> +      MOZ_ASSERT(other.prev->next == &other);
> +
> +      other.next->prev = this;
> +      other.prev->next = this;

I'd do

  next->prev = this;
  prev->next = this;

instead, as you're really mutating fields in *this* element's next/prev to connect up to this -- reads slightly conceptually more clearly to me.

@@ +135,5 @@
> +      other.next->prev = this;
> +      other.prev->next = this;
> +
> +      other.next = &other;
> +      other.prev = &other;

Before this, I'd do a symmetric comment to the one above:

  /* Adjust |other| to destructible, not-in-list status. */

Comment 17

[Jeff Walden [:Waldo]]

Comment on attachment 792880 [details] [diff] [review]
Patch, v2 (now with C++11 move)

Mm, Bugzilla fail when toggling flags on an obsoleted patch.  Looks like comments transfer over trivially, so no big deal here.

Comment 18

[Justin Lebar (not reading bugmail)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3f307f37b77a

Thanks for all the reviews lately.

Comment 19

[Jeff Walden [:Waldo]]

(In reply to Justin Lebar [:jlebar] from comment #18)
> Thanks for all the reviews lately.

https://twitter.com/sayrer/status/19304989209

Comment 20

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/3f307f37b77a