909977 - Rename mozilla::Move to mozilla::MozMove and make mozilla::Move a synonym for std::move

Status: RESOLVED FIXED

(Core :: MFBT, defect)

Description

[Justin Lebar (not reading bugmail)]

Per the discussion in bug 896100.

This is super-ugly.  We can't get rid of mozilla::Move() fast enough, IMO.

Comment 1

[Justin Lebar (not reading bugmail)]

Attachment: Patch, v1

Comment 2

[Jeff Walden [:Waldo]]

Comment on attachment 796312 [details] [diff] [review]
Patch, v1

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

This was a nice, relatively bite-sized bit of Move reviewing -- not too little, not too much.  :-)

::: gfx/thebes/gfxFont.h
@@ +606,5 @@
>          FontTableHashEntry(KeyTypePointer aTag)
>              : KeyClass(aTag), mBlob() { }
> +
> +        FontTableHashEntry(FontTableHashEntry&& toMove)
> +            : KeyClass(toMove), mBlob(toMove.mBlob)

Add mozilla::Move around both of these to follow the general moving pattern.

I could be mistaken, but I think this constructor would be more optimal -- and possibly more correct, although if the existing code is buggy I'm not sure how that wouldn't have been noticed before -- if it moved mSharedBlobData as well as mBlob, and if it nulled out the latter in the body.  The rule of thumb for move-construction should be that destructing the object-to-be-moved should be a no-op.  But right now, the constructor calls Clear(), and that does stuff if |mSharedBlobData|.  So for the destructor to be a no-op, both fields have to be null.

I think this bit probably should be reviewed by a gfx person to double-check me on this.

::: mfbt/Move.h
@@ +17,5 @@
>   * "Move" References
>   *
> + * [Once upon a time, C++11 rvalue references were not implemented by all the
> + * compilers we cared about, so we invented mozilla::Move() (now called
> + * MozMove()), which does something similar.  We're in the process of

I would name it OldMove (or even BadMove), which is slightly more suggestive that it shouldn't be used.  But if it's much trouble sed'ing your patch queues or whatever, no big deal.  Presumably the old thing will disappear fast enough it won't much matter.  (Famous last words.)

@@ +66,3 @@
>   *
> + * The Move(T&) function takes a reference to a T and returns a T&&.  It acts
> + * just like stl::move(), which is not available on all our platforms.

Pervasive typo: s/stl/std/g

@@ +183,5 @@
> +inline T&&
> +Forward(typename RemoveReference<T>::Type& a)
> +{
> +  return static_cast<T&&>(a);
> +}

C++11 says you also need

template<typename T>
inline T&&
Forward(typename RemoveReference<T>::Type&& t)
{
  static_assert(!IsLvalueReference<T>::value,
                "misuse of Forward detected!  try the other overload");
  return static_cast<T&&>(t);
}

which I *think* is required to correctly forward rvalue references.  And of course this (modulo typos) in TypeTraits.h to make it work:

template<typename T>
struct IsLvalueReference : FalseType {};

template<typename T>
struct IsLvalueReference<T&> : TrueType {};

@@ +200,5 @@
>  /** Swap |t| and |u| using move-construction if possible. */
>  template<typename T>
>  inline void
>  Swap(T& t, T& u)
>  {

Hum.  Not sure if this should stay as-is, or be converted to rvalue references now, yet.  Safest would be adding OldSwap as well.  If this doesn't break stuff, it's good enough for now.

::: mfbt/TypeTraits.h
@@ +485,5 @@
> + * mozilla::RemoveReference<A&>::Type is A;
> + * mozilla::RemoveReference<A&&>::Type is A;
> + */
> +
> +template<class A>

typename T (typename because it's not necessarily a "class", T being slightly more canonical), please (same for the others).

::: xpcom/glue/nsBaseHashtable.h
@@ +413,5 @@
>  template<class KeyClass,class DataType>
>  nsBaseHashtableET<KeyClass,DataType>::nsBaseHashtableET
> +  (nsBaseHashtableET<KeyClass,DataType>&& toMove) :
> +  KeyClass(toMove),
> +  mData(mozilla::Move(toMove.mData))

I think you want to mozilla::Move(toMove) as well, as otherwise you get copy-construction of the base class rather than move-construction.  It could be slightly more efficient, depending on the key type, to move the base here, I think.

::: xpcom/glue/nsTHashtable.h
@@ +379,3 @@
>  nsTHashtable<EntryType>::nsTHashtable()
>  {
>    // entrySize is our "I'm initialized" indicator

Minor tangential-ish nitpick, but it'd be nice to see |entrySize == sizeof(EntryType)| (as set by nsTHashtable<EntryType>::Init via PL_DHashTableInit) stated to be the I'm-initialized indicator (and |entrySize == 0| being the I'm-not-initialized indicator.  More explicitness about the state of something initialized, versus the state of something uninitialized, would have helped me with verifying this stuff.

@@ +389,2 @@
>  {
> +  aOther.mTable = PLDHashTable();

There's no need to overwrite all of mTable here (it seems according to http://stackoverflow.com/questions/563221/is-there-an-implicit-default-constructor-in-c and what I skimmed not-entirely-confidently of C++11, this completely zeroes aOther.mTable) -- just the entrySize bit.  The thing here is fine enough, but not optimal.

How about we use MOZ_MAKE_MEM_UNDEFINED(addr, size) from mozilla/MemoryChecking.h to make this a little quicker?  I'd probably do it in a secondary push, to keep any triggered valgrind errors better-distinguishable.

@@ +456,5 @@
>  }
>  
>  template<class EntryType>
>  void
>  nsTHashtable<EntryType>::s_CopyEntry(PLDHashTable          *table,

s_CopyEntry?  Love that naming.

@@ +468,2 @@
>  
>    fromEntry->~EntryType();

Hmm.  Looking at pldhash.cpp, it looks like this definitely does need to destruct |fromEntry|, because ChangeTable never does so itself.  Those semantics seem under-documented to me in pldhash.h.  Could you file a followup to expand the comment to say that PLDHashMoveEntry must uninitialize |from|?

Comment 3

[Justin Lebar (not reading bugmail)]

> s_CopyEntry?  Love that naming.

p_What v_do p_you v_have p_against a_Hungarian n_notation??

Comment 4

[Justin Lebar (not reading bugmail)]

> I would name it OldMove (or even BadMove), which is slightly more suggestive
> that it shouldn't be used.  But if it's much trouble sed'ing your patch
> queues or whatever, no big deal. 

I can do OldMove.

> C++11 says you also need

The rvalue references tutorial strongly suggests to me that we don't need this,
due to ref collapsing and the special template rule he talks about.  He
explicitly says that the function in the patch will forward rvalues.

http://thbecker.net/articles/rvalue_references/section_08.html

But of course the tutorial could be wrong, or simplifying something.  Maybe we
should simply not include mozilla::Forward until we have someone who wants to
use it?

Comment 5

[Justin Lebar (not reading bugmail)]

> I think this constructor would be more optimal -- and possibly more correct, although if the 
> existing code is buggy I'm not sure how that wouldn't have been noticed before -- if it moved 
> mSharedBlobData as well as mBlob, and if it nulled out the latter in the body.  The rule of thumb 
> for move-construction should be that destructing the object-to-be-moved should be a no-op.  But 
> right now, the constructor calls Clear(), and that does stuff if |mSharedBlobData|.  So for the 
> destructor to be a no-op, both fields have to be null.

Yeah, something is really weird in that class.  But I'd rather not change its behavior without input from the relevant people.  I filed bug 910376.

Comment 6

[Justin Lebar (not reading bugmail)]

> Add mozilla::Move around both of these to follow the general moving pattern.

So the pattern is to move() even primitives (e.g. pointers?).  I guess that's sensible...

Comment 7

[Justin Lebar (not reading bugmail)]

(Sorry to keep spamming.)

I guess the reason I didn't like Move()'ing the mBlob pointer is that it gives you a false sense of security:  Even if you Move() the pointer, you still have to do |toMove.mBlob = nullptr|.

I'll change it like you asked, but just something to keep in mind.

Comment 8

[Justin Lebar (not reading bugmail)]

> Could you file a followup to expand the comment to say that PLDHashMoveEntry must uninitialize 
> |from|?

Filed bug 910388.

Comment 9

[Jeff Walden [:Waldo]]

(In reply to Justin Lebar [:jlebar] from comment #7)
> I guess the reason I didn't like Move()'ing the mBlob pointer is that it
> gives you a false sense of security:  Even if you Move() the pointer, you
> still have to do |toMove.mBlob = nullptr|.
> 
> I'll change it like you asked, but just something to keep in mind.

A good point.  I've been assuming the typical pattern would be to move everything, but that could be wrong.  Luke, do you know anything about the recommended move-constructor idioms for this stuff?

Comment 10

[Luke Wagner [:luke]]

I've never bothered moving primitives, but I can't speak to knowing any best practices.  On style I like is: if you provide move, delete copy/assign, that way if anyone forgets they don't silently fall back on copy.  With that, then there is not such a perf danger of forgetting to Move a field.

Comment 11

[Jeff Walden [:Waldo]]

(In reply to Justin Lebar [:jlebar] from comment #4)
> The rvalue references tutorial strongly suggests to me that we don't need
> this, due to ref collapsing and the special template rule he talks about.
> He explicitly says that the function in the patch will forward rvalues.
> 
> http://thbecker.net/articles/rvalue_references/section_08.html
> 
> But of course the tutorial could be wrong, or simplifying something.  Maybe
> we should simply not include mozilla::Forward until we have someone who
> wants to use it?

I think he's probably wrong about this.  The C++11 spec is very clear about there being two overloads here, as is <utility> on my system.  We should follow it.

I know a number of different mozilla::Forward users that will show up in very short order once that lands, so we should go ahead with it now while it's fresh in memory.

Comment 12

[Justin Lebar (not reading bugmail)]

Thanks, Jeff.  I landed this with the Forward stuff as you described.

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

Comment 13

[Ed Morley [:emorley]]

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