1405483 - forbid const-qualified types from being passed to Move()

Status: NEW

(Core :: MFBT, enhancement)

Description

[Nathan Froyd [:froydnj]]

I fixed bug 1405348 today, where we had code like:

  typedef const Type& KeyType;

  ...

  KeyType GetKey() const { return mKey; }

  ...

  ConstructorOfJustice(ConstructorOfJustice&& aOther)
    : mKey(Move(aOther.GetKey()))

so the code sort of looks like it does the efficient thing, but really turns out to do the inefficient thing, because Move() on const-qualified types winds up returning `const T&&`, which makes no sense and will select the copy constructor of the type during overload resolution, rather than the move constructor that was desired.

WebKit has their own WTF::move with this check and an additional check for non-lvalue references:

https://trac.webkit.org/browser/webkit/trunk/Source/WTF/wtf/StdLibExtras.h#L514

This bug is *just* for the const check; I think compilers might be able to deal with non-lvalue references just fine, although it might be worth following up to add the non-lvalue check as well.

Adding the obvious static assert to Move() turns up way more instances that you would expect...though most of them appear to be related to templates (Variant and JS hash tables, it looks like), which heightens the amount of warning spam you receive.

Opening this bug to either fix everything, or to hang off dependent bugs for fixing things one-by-one.

Comment 1

[Nika Layzell [:nika] (ni? for response)]

I think this is generally a good idea.

One other option which I feel I should bring up, is that if we're wanting to only error in situations where the source type is known (as-in it's not occurring within templated types), then we could also use a clang static analysis. This would let us only find & complain about those locations, in exchange for a worse developer feedback story.

The other option would be to make mozilla::Move act in the new way, and change templated consumers to instead use a mozilla::GenericMove which doesn't perform the assertion, as it is intended to be used in generic contexts.

Comment 2

[Firefox Bug Husbandry Bot]

https://wiki.mozilla.org/Bug_Triage/Projects/Bug_Handling/Bug_Husbandry#Update_affecteds