-Wpessimizing-move and -Wredundant-move

# Michael Catanzaro (2 days ago)

GCC 9 has new -Wpessimizing-move ("warning: moving a local object in a return statement prevents copy elision") and -Wredundant-move ("warning: redundant move in return statement") warnings. These are enabled by -Wextra (which we use) and will be triggered by code like:

return WTFMove(foo);

when foo is a copyable type. This idiom is only appropriate if foo is a noncopyable (move-only) type. We have many, many instances of such code. I'm trying to fix them. Please don't add more! (It's easy enough to disable the warnings, but they're there for a reason.)

Thanks,

Michael

Contact us to advertise here
# Xan (2 days ago)

I recently had to test GCC 9 and fixed one of the worst offenders in terms of number of warnings when compiling JSC. See: bugs.webkit.org/show_bug.cgi?id=195798

# Andy Estes (2 days ago)

On Mar 18, 2019, at 2:35 PM, Michael Catanzaro <mcatanzaro at igalia.com> wrote:

Hi,

GCC 9 has new -Wpessimizing-move ("warning: moving a local object in a return statement prevents copy elision") and -Wredundant-move ("warning: redundant move in return statement") warnings. These are enabled by -Wextra (which we use) and will be triggered by code like:

return WTFMove(foo);

when foo is a copyable type. This idiom is only appropriate if foo is a noncopyable (move-only) type. We have many, many instances of such code. I'm trying to fix them. Please don't add more! (It's easy enough to disable the warnings, but they're there for a reason.)

It might not even be appropriate for move-only types because of NRVO.

FWIW, Apple’s ports use the equivalent clang warning for pessimizing and redundant moves, and we cleaned up a bunch of these mistakes in our ports a few years ago. Hopefully you aren’t finding any of these mistakes in shared code.

(WTFMove was written to accommodate these warnings, in fact.)

Really glad you’re turning these warnings on!

Andy

# Michael Catanzaro (2 days ago)

On Mon, Mar 18, 2019 at 4:43 PM, Andy Estes <aestes at apple.com> wrote:

FWIW, Apple’s ports use the equivalent clang warning for pessimizing and redundant moves, and we cleaned up a bunch of these mistakes in our ports a few years ago. Hopefully you aren’t finding any of these mistakes in shared code.

There are a lot, actually. Maybe clang only has -Wpessimizing-move (which caused relatively few warnings) and not -Wredundant-move (which caused very many). I'm not actually sure what the difference is; in both cases, a local variable is cast to rvalue reference via std::move before it is returned.

Anyway: patch in bugs.webkit.org/show_bug.cgi?id=195920, review appreciated. That patch also fixes several -Wdeprecated-copy warnings and a couple -Wclass-memaccess, but the vast majority is just removing WTFMove().

(WTFMove was written to accommodate these warnings, in fact.)

Really glad you’re turning these warnings on!

Note we actually haven't changed our warning flags, they're just new warnings enabled by -Wextra, and we've always used -Wextra. So let's thank the GCC developers. Each new GCC is a ruined day for me whenever I try to make WebKit build cleanly, but the warnings are worth it.

Michael

# Tomas Popela (2 days ago)

On Mon, Mar 18, 2019 at 10:40 PM Michael Catanzaro <mcatanzaro at igalia.com>

wrote:

-Wredundant-move ("warning: redundant move in return statement") warnings.

If a note from cgit.freedesktop.org/libreoffice/core/commit/?id=dc06c8f4989fc28d0c31ebd333e53dfe0e0f5f66 applies, then you can't fix it until we support Ubuntu 14.04 (due to its old gcc version):

Turns out the std::move can only be dropped if the compiler has a fix for CWG1579. For GCC that's the case starting with GCC 5.1

Tom

# Michael Catanzaro (2 days ago)

On Tue, Mar 19, 2019 at 12:57 AM, Tomas Popela <tpopela at redhat.com>

wrote:

If a note from cgit.freedesktop.org/libreoffice/core/commit/?id=dc06c8f4989fc28d0c31ebd333e53dfe0e0f5f66 applies, then you can't fix it until we support Ubuntu 14.04 (due to its old gcc version):

Turns out the std::move can only be dropped if the compiler has a fix for CWG1579.
For GCC that's the case starting with GCC 5.1

Currently we require GCC 6 to build, and if we follow our dependencies policy we'll be able to require GCC 7 next month, so that should be no problem.

# Tomas Popela (2 days ago)

Ah, good to know! Thank you Michael :)

Tom

On Tue, Mar 19, 2019 at 1:42 PM Michael Catanzaro <mcatanzaro at igalia.com>

wrote:

# Emilio Cobos Álvarez (2 days ago)

On 19/03/2019 00:56, Michael Catanzaro wrote:

On Mon, Mar 18, 2019 at 4:43 PM, Andy Estes <aestes at apple.com> wrote:

FWIW, Apple’s ports use the equivalent clang warning for pessimizing and redundant moves, and we cleaned up a bunch of these mistakes in our ports a few years ago. Hopefully you aren’t finding any of these mistakes in shared code.

There are a lot, actually. Maybe clang only has -Wpessimizing-move (which caused relatively few warnings) and not -Wredundant-move (which caused very many). I'm not actually sure what the difference is; in both cases, a local variable is cast to rvalue reference via std::move before it is returned.

IIRC, clang only warns when using std::move, not a similarly defined function (like WTFMove, presumably).

In Gecko, when I switched from mozilla::Move to std::move 1, I had to disable the warning and fix all of them in a followup, since clang didn't use to warn about them.

If gcc is able to warn more generally, that may explain what you're seeing.

-- Emilio

# Michael Catanzaro (2 days ago)

Several more return WTFMove() cases were committed just between yesterday and today. Please be careful. :)

On Tue, Mar 19, 2019 at 8:01 AM, Emilio Cobos =?iso-8859-1?q?=C1lvarez?= <emilio at crisal.io> wrote:

In Gecko, when I switched from mozilla::Move to std::move [1], I had to disable the warning and fix all of them in a followup, since clang didn't use to warn about them.

We used to have WTF::move defined as a function, but now it's a #define to ensure std::move gets called directly. I think that's what Andy meant when he says "WTFMove was written to accommodate these warnings."

Michael

Want more features?

Request early access to our private beta of readable email premium.