Re: [webkit-dev] -Wpessimizing-move and -Wredundant-move
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?= 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 ___ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] -Wpessimizing-move and -Wredundant-move
On 19/03/2019 00:56, Michael Catanzaro wrote: > On Mon, Mar 18, 2019 at 4:43 PM, Andy Estes 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 [1]: https://bugzilla.mozilla.org/show_bug.cgi?id=1465585 ___ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] -Wpessimizing-move and -Wredundant-move
Ah, good to know! Thank you Michael :) Tom On Tue, Mar 19, 2019 at 1:42 PM Michael Catanzaro wrote: > On Tue, Mar 19, 2019 at 12:57 AM, Tomas Popela > wrote: > > If a note from > > > https://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. > > > ___ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] -Wpessimizing-move and -Wredundant-move
On Tue, Mar 19, 2019 at 12:57 AM, Tomas Popela wrote: If a note from https://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. ___ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] -Wpessimizing-move and -Wredundant-move
Hi,
On Mon, Mar 18, 2019 at 10:40 PM Michael Catanzaro
wrote:
> -Wredundant-move ("warning: redundant move in return statement") warnings.
>
If a note from
https://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
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] -Wpessimizing-move and -Wredundant-move
On Mon, Mar 18, 2019 at 4:43 PM, Andy Estes 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 https://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 ___ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] -Wpessimizing-move and -Wredundant-move
Hi Michael,
> On Mar 18, 2019, at 2:35 PM, Michael Catanzaro 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
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] -Wpessimizing-move and -Wredundant-move
I recently had to test GCC 9 and fixed one of the worst offenders in terms
of number of warnings when compiling JSC. See:
https://bugs.webkit.org/show_bug.cgi?id=195798
On Mon, Mar 18, 2019, 22:36 Michael Catanzaro 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.)
>
> Thanks,
>
> Michael
>
>
> ___
> webkit-dev mailing list
> webkit-dev@lists.webkit.org
> https://lists.webkit.org/mailman/listinfo/webkit-dev
>
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev
