Bug 152557 – Stop moving local objects in return statements

• Component: New Bugs
• Status: RESOLVED FIXED

[Andy Estes]

Stop moving local objects in return statements

[Andy Estes]

Created attachment 267915 [details]
Patch

[Andy Estes]

Created attachment 267920 [details]
Patch

[Brady Eidson]

Comment on attachment 267920 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=267920&action=review

Can you resubmit for the commit queue to see if mac-debug clears up?

> Source/JavaScriptCore/ChangeLog:11
> +        I found these issues by temporarily replacing WTF::move with WTF::move and recompiling.

"WTF::move" and "WTF::move" are the same string, so I suspect there's a typo in your ChangeLog.

[Andy Estes]

(In reply to comment #3)
> Comment on attachment 267920 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=267920&action=review
> 
> Can you resubmit for the commit queue to see if mac-debug clears up?

Sure, I'll try.

> 
> > Source/JavaScriptCore/ChangeLog:11
> > +        I found these issues by temporarily replacing WTF::move with WTF::move and recompiling.
> 
> "WTF::move" and "WTF::move" are the same string, so I suspect there's a typo
> in your ChangeLog.

Hah, this ChangeLog is in the JSC Xcode project for some reason, so this got swept up in my global replace of std::move back to WTF::move.

[Alexey Proskuryakov]

There is no need to re-submit for EWS, it keeps trying. You can learn what's going on by hovering the bubble, or by clicking on it.

Someone probably landed a bad patch while no one was watching.

[Alexey Proskuryakov]

Hmm, in fact it's because bot ews113 went crazy, and somehow, each attempt ran on the same bot. Something is wrong here, but for now, I just rebooted it.

[Brady Eidson]

OH OH OH!

This hasn't landed yet, good!

Because I thought of a super obvious improvement: Make check-webkit-style warn for "return WTF::move("

[Andy Estes]

(In reply to comment #7)
> OH OH OH!
> 
> This hasn't landed yet, good!
> 
> Because I thought of a super obvious improvement: Make check-webkit-style
> warn for "return WTF::move("

There's at least one case where (I believe) we have to move a return value:

class Base {
};

class Derived : public Base {
};

std::unique_ptr<Base> create()
{
    auto derived { std::make_unique<Derived>() };
    return WTF::move(derived);
}

Since the type of create()'s return value differs from the type of the local, this function is not eligible for the RVO, and since std::unique_ptr<> is a move-only type, it can't be copied into the return value.

There's also one more case where it's acceptable (although not mandatory) to move a return value, and that's a member function that releases a member variable to the caller (although I'd prefer we use std::exchange() for those cases).

[Andy Estes]

Committed r194428: <http://trac.webkit.org/changeset/194428>