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

RESOLVED FIXED in mozilla26

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: justin.lebar+bug, Assigned: justin.lebar+bug)

Tracking

(Blocks 1 bug)

Trunk
mozilla26
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

This is helpful in general and in particular for WIP patches in bug 901746.
Blocks: 901746
Assignee: nobody → justin.lebar+bug
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.
Attachment #792466 - Flags: review?(jwalden+bmo)
It would be great if you could use C++11 move constructors instead.  The fewer usages of mozilla::Move we add, the better.
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?).
> 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 on attachment 792466 [details] [diff] [review]
Part 1, v1: Add mozilla::Move::ptr().

Yeah, this is much better using proper C++11.
Attachment #792466 - Attachment is obsolete: true
Attachment #792466 - Flags: review?(jwalden+bmo)
Attachment #792467 - Attachment is obsolete: true
Attachment #792467 - Flags: review?(jwalden+bmo)
Attachment #792880 - Flags: review?(jwalden+bmo)
Depends on: 896100
(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.
otoh we need to land something so that there's an alternative to std::move(), right?
(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.)
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...
(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.)
Rename the existing one to DeprecatedMove or something, then.  It's going away, make it ugly and obvious that no one should use it.
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.
Posted patch Patch, v3Splinter Review
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.
Attachment #792880 - Attachment is obsolete: true
Attachment #792880 - Flags: review?(jwalden+bmo)
Attachment #796309 - Flags: review?(jwalden+bmo)
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. */
Attachment #792880 - Attachment is obsolete: false
Attachment #796309 - Flags: review?(jwalden+bmo) → review+
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.
Attachment #792880 - Attachment is obsolete: true
(In reply to Justin Lebar [:jlebar] from comment #18)
> Thanks for all the reviews lately.

https://twitter.com/sayrer/status/19304989209
https://hg.mozilla.org/mozilla-central/rev/3f307f37b77a
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
You need to log in before you can comment on or make changes to this bug.