Bug 152601 - Use of WTF::move prevents clang's move diagnostics from warning about several classes of mistakes
Summary: Use of WTF::move prevents clang's move diagnostics from warning about several...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Template Framework (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Andy Estes
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-12-30 14:36 PST by Andy Estes
Modified: 2016-01-04 09:25 PST (History)
19 users (show)

See Also:


Attachments
I. Add std::move<WTF::CheckMoveParameter> and WTF_MOVE (2.92 KB, patch)
2015-12-30 15:21 PST, Andy Estes
no flags Details | Formatted Diff | Diff
I. Add std::move<WTF::CheckMoveParameter> and WTF_MOVE (2.90 KB, patch)
2015-12-30 15:41 PST, Andy Estes
no flags Details | Formatted Diff | Diff
II. Fix warnings uncovered by migrating to WTF_MOVE (8.79 KB, patch)
2015-12-31 12:40 PST, Andy Estes
no flags Details | Formatted Diff | Diff
III. Rename WTF_MOVE to WTFMove (1.28 KB, patch)
2015-12-31 12:46 PST, Andy Estes
no flags Details | Formatted Diff | Diff
IV. Update the style checker to advise using WTFMove() instead of WTF::move() (2.85 KB, patch)
2015-12-31 13:22 PST, Andy Estes
no flags Details | Formatted Diff | Diff
V. Replace WTF::move with WTFMove (1.75 MB, patch)
2015-12-31 19:54 PST, Andy Estes
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andy Estes 2015-12-30 14:36:26 PST
Clang has recently added warnings to catch certain classes of mistakes with the use of std::move(): -Wpessimizing-move (warns if moving prevents copy elision), -Wredundant-move (warns if a move is redundant), and -Wself-move (warns if moving to self). Enabling these warnings manually (by renaming WTF::move to std::move) have caught numerous mistakes in our codebase (see http://trac.webkit.org/changeset/194428).

It would be nice to be able to take advantage of these warnings, but doing so requires that we use std::move, not WTF::move. But since WTF::move does provide useful checks for which clang does not yet have warnings, we should find a way to write a best-of-both-worlds move function. I think we could do this like so:

    namespace WTF {
    enum CheckMoveParameterTag { CheckMoveParameter };
    }

    namespace std {
    template<WTF::CheckMoveParameterTag, typename T>
    ALWAYS_INLINE CONSTEXPR typename remove_reference<T>::type&& move(T&& value)
    {
        // static_asserts from WTF::move
        return std::move(std::forward<T>(value));
    }
    }

    #define WTF_MOVE(value) std::move<WTF::CheckMoveParameter>(value)

This move function satisfies clang's criteria for a move function (in namespace std, called "move", takes a single argument), but allows us to add our own compile-time asserts to check for moving from const and moving from rvalue reference. A macro called WTF_MOVE() is added for convenience.

I plan to address this in two stages:

1. Upload a patch that defines std::move<WTF::CheckMoveParameter>() and WTF_MOVE().
2. Upload a patch that removes WTF::move and renames all uses of WTF::move to WTF_MOVE.
Comment 1 Andy Estes 2015-12-30 15:21:12 PST
Created attachment 268014 [details]
I. Add std::move<WTF::CheckMoveParameter> and WTF_MOVE
Comment 2 Andy Estes 2015-12-30 15:41:14 PST
Created attachment 268016 [details]
I. Add std::move<WTF::CheckMoveParameter> and WTF_MOVE
Comment 3 WebKit Commit Bot 2015-12-30 21:24:33 PST
Comment on attachment 268016 [details]
I. Add std::move<WTF::CheckMoveParameter> and WTF_MOVE

Clearing flags on attachment: 268016

Committed r194451: <http://trac.webkit.org/changeset/194451>
Comment 4 WebKit Commit Bot 2015-12-30 21:24:39 PST
All reviewed patches have been landed.  Closing bug.
Comment 5 Andy Estes 2015-12-30 22:35:48 PST
Reopening to add part 2.
Comment 6 Daniel Bates 2015-12-30 22:54:06 PST
(In reply to comment #0)
> [...]
> 1. Upload a patch that defines std::move<WTF::CheckMoveParameter>() and
> WTF_MOVE().

I know that this part was already reviewed and landed. As per <https://webkit.org/code-style-guidelines/#names-define-non-const> we use similar naming and style conventions for macro functions as we do for non-macro functions. According to that style rule we should use camel case and WTFMove() seems to be acceptable. Though, according to <https://webkit.org/code-style-guidelines/#names-basic> wtfMove() would be acceptable :/
Comment 7 Daniel Bates 2015-12-30 22:59:33 PST
Additionally we should update the style checker to advise using WTF_MOVE() instead of WTF::move(). See check_wtf_move() in <http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/style/checkers/cpp.py?rev=192274#L2258>.
Comment 8 Andy Estes 2015-12-30 23:00:16 PST
(In reply to comment #6)
> (In reply to comment #0)
> > [...]
> > 1. Upload a patch that defines std::move<WTF::CheckMoveParameter>() and
> > WTF_MOVE().
> 
> I know that this part was already reviewed and landed. As per
> <https://webkit.org/code-style-guidelines/#names-define-non-const> we use
> similar naming and style conventions for macro functions as we do for
> non-macro functions. According to that style rule we should use camel case
> and WTFMove() seems to be acceptable. Though, according to
> <https://webkit.org/code-style-guidelines/#names-basic> wtfMove() would be
> acceptable :/

It's not too late to change the name, and I'm not tied to this particular name. Personally, I like being able to distinguish between macros and functions, but you're right that our style guidelines disagree with me.

I don't object to WTFMove(), and I think I like that better than wtfMove().
Comment 9 Andy Estes 2015-12-30 23:00:59 PST
(In reply to comment #7)
> Additionally we should update the style checker to advise using WTF_MOVE()
> instead of WTF::move(). See check_wtf_move() in
> <http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/style/checkers/
> cpp.py?rev=192274#L2258>.

I was aware of this, and plan to address it in the next patch that renames everything.
Comment 10 Andy Estes 2015-12-31 12:40:27 PST
Created attachment 268050 [details]
II. Fix warnings uncovered by migrating to WTF_MOVE
Comment 11 Andy Estes 2015-12-31 12:46:08 PST
Created attachment 268051 [details]
III. Rename WTF_MOVE to WTFMove
Comment 12 Andy Estes 2015-12-31 13:22:52 PST
Created attachment 268052 [details]
IV. Update the style checker to advise using WTFMove() instead of WTF::move()
Comment 13 WebKit Commit Bot 2015-12-31 14:47:35 PST
Comment on attachment 268051 [details]
III. Rename WTF_MOVE to WTFMove

Clearing flags on attachment: 268051

Committed r194469: <http://trac.webkit.org/changeset/194469>
Comment 14 WebKit Commit Bot 2015-12-31 14:49:29 PST
Comment on attachment 268050 [details]
II. Fix warnings uncovered by migrating to WTF_MOVE

Clearing flags on attachment: 268050

Committed r194470: <http://trac.webkit.org/changeset/194470>
Comment 15 WebKit Commit Bot 2015-12-31 14:52:00 PST
Comment on attachment 268052 [details]
IV. Update the style checker to advise using WTFMove() instead of WTF::move()

Clearing flags on attachment: 268052

Committed r194471: <http://trac.webkit.org/changeset/194471>
Comment 16 WebKit Commit Bot 2015-12-31 14:52:07 PST
All reviewed patches have been landed.  Closing bug.
Comment 17 Andy Estes 2015-12-31 14:54:23 PST
Reopening again to remove WTF::move and do the WTF::move-to-WTFMove rename.
Comment 18 Ryan Haddad 2015-12-31 16:16:41 PST
(In reply to comment #17)
> Reopening again to remove WTF::move and do the WTF::move-to-WTFMove rename.

Forgive me if this will be addressed by the change you mention here, but it looks like the Yosemite builds are failing with:

/Volumes/Data/slave/yosemite-32bit-release/build/Tools/TestWebKitAPI/Tests/WTF/NakedPtr.cpp:184:34: error: unknown warning group '-Wself-move', ignored [-Werror,-Wunknown-pragmas]

<https://build.webkit.org/builders/Apple%20Yosemite%20Release%20%2832-bit%20Build%29/builds/8475>
Comment 19 David Kilzer (:ddkilzer) 2015-12-31 16:21:28 PST
(In reply to comment #18)
> (In reply to comment #17)
> > Reopening again to remove WTF::move and do the WTF::move-to-WTFMove rename.
> 
> Forgive me if this will be addressed by the change you mention here, but it
> looks like the Yosemite builds are failing with:
> 
> /Volumes/Data/slave/yosemite-32bit-release/build/Tools/TestWebKitAPI/Tests/
> WTF/NakedPtr.cpp:184:34: error: unknown warning group '-Wself-move', ignored
> [-Werror,-Wunknown-pragmas]
> 
> <https://build.webkit.org/builders/Apple%20Yosemite%20Release%20%2832-
> bit%20Build%29/builds/8475>

Build fix to address the above failure on Yosemite:

<https://trac.webkit.org/r194473>
Comment 20 David Kilzer (:ddkilzer) 2015-12-31 16:22:47 PST
Oops, Andy had one more step per Comment #17.
Comment 21 Andy Estes 2015-12-31 16:24:24 PST
(In reply to comment #19)
> (In reply to comment #18)
> > (In reply to comment #17)
> > > Reopening again to remove WTF::move and do the WTF::move-to-WTFMove rename.
> > 
> > Forgive me if this will be addressed by the change you mention here, but it
> > looks like the Yosemite builds are failing with:
> > 
> > /Volumes/Data/slave/yosemite-32bit-release/build/Tools/TestWebKitAPI/Tests/
> > WTF/NakedPtr.cpp:184:34: error: unknown warning group '-Wself-move', ignored
> > [-Werror,-Wunknown-pragmas]
> > 
> > <https://build.webkit.org/builders/Apple%20Yosemite%20Release%20%2832-
> > bit%20Build%29/builds/8475>
> 
> Build fix to address the above failure on Yosemite:
> 
> <https://trac.webkit.org/r194473>

Thanks Dave!
Comment 22 Andy Estes 2015-12-31 19:54:35 PST
Created attachment 268064 [details]
V. Replace WTF::move with WTFMove
Comment 23 WebKit Commit Bot 2015-12-31 19:58:40 PST
Attachment 268064 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/text/AtomicString.h:83:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WebCore/css/RuleSet.h:150:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WebKit2/Scripts/webkit/LegacyMessageReceiver-expected.cpp:77:  _result is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebKit2/Scripts/webkit/LegacyMessageReceiver-expected.cpp:96:  _result is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/bytecode/GetByIdVariant.cpp:43:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/WebCore/accessibility/AccessibilityObject.h:251:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/PolymorphicCallStubRoutine.cpp:75:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/WebKit/mac/WebCoreSupport/WebFrameLoaderClient.mm:876:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKit/mac/WebCoreSupport/WebFrameLoaderClient.mm:908:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKit/mac/WebCoreSupport/WebFrameLoaderClient.mm:920:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/bindings/js/JSDOMPromise.h:73:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WebCore/bindings/js/JSDOMPromise.h:74:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WebCore/platform/cocoa/ContentFilterUnblockHandlerCocoa.mm:61:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WebCore/rendering/style/DataRef.h:33:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WebCore/css/Pair.h:67:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WebCore/css/Pair.h:68:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
ERROR: Source/JavaScriptCore/bytecode/StructureStubClearingWatchpoint.h:53:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/WebKit2/Scripts/webkit/MessageReceiver-expected.cpp:77:  _result is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebKit2/Scripts/webkit/MessageReceiver-expected.cpp:96:  _result is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 20 in 1206 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 24 Andy Estes 2015-12-31 20:01:55 PST
(In reply to comment #23)
> Attachment 268064 [details] did not pass style-queue:
> 
> 
> ERROR: Source/WTF/wtf/text/AtomicString.h:83:  Should be indented on a
> separate line, with the colon or comma first on that line. 
> [whitespace/indent] [4]
> ERROR: Source/WebCore/css/RuleSet.h:150:  Should be indented on a separate
> line, with the colon or comma first on that line.  [whitespace/indent] [4]
> ERROR: Source/WebKit2/Scripts/webkit/LegacyMessageReceiver-expected.cpp:77: 
> _result is incorrectly named. Don't use underscores in your identifier
> names.  [readability/naming/underscores] [4]
> ERROR: Source/WebKit2/Scripts/webkit/LegacyMessageReceiver-expected.cpp:96: 
> _result is incorrectly named. Don't use underscores in your identifier
> names.  [readability/naming/underscores] [4]
> ERROR: Source/JavaScriptCore/bytecode/GetByIdVariant.cpp:43:  Wrong number
> of spaces before statement. (expected: 8)  [whitespace/indent] [4]
> ERROR: Source/WebCore/accessibility/AccessibilityObject.h:251:  Wrong number
> of spaces before statement. (expected: 8)  [whitespace/indent] [4]
> ERROR: Source/JavaScriptCore/jit/PolymorphicCallStubRoutine.cpp:75:  Wrong
> number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
> ERROR: Source/WebKit/mac/WebCoreSupport/WebFrameLoaderClient.mm:876:  Weird
> number of spaces at line-start.  Are you using a 4-space indent? 
> [whitespace/indent] [3]
> ERROR: Source/WebKit/mac/WebCoreSupport/WebFrameLoaderClient.mm:908:  Weird
> number of spaces at line-start.  Are you using a 4-space indent? 
> [whitespace/indent] [3]
> ERROR: Source/WebKit/mac/WebCoreSupport/WebFrameLoaderClient.mm:920:  Weird
> number of spaces at line-start.  Are you using a 4-space indent? 
> [whitespace/indent] [3]
> ERROR: Source/WebCore/bindings/js/JSDOMPromise.h:73:  Should be indented on
> a separate line, with the colon or comma first on that line. 
> [whitespace/indent] [4]
> ERROR: Source/WebCore/bindings/js/JSDOMPromise.h:74:  Should be indented on
> a separate line, with the colon or comma first on that line. 
> [whitespace/indent] [4]
> ERROR: Source/WebCore/platform/cocoa/ContentFilterUnblockHandlerCocoa.mm:61:
> Code inside a namespace should not be indented.  [whitespace/indent] [4]
> ERROR: Source/WebCore/rendering/style/DataRef.h:33:  Should be indented on a
> separate line, with the colon or comma first on that line. 
> [whitespace/indent] [4]
> ERROR: Source/WebCore/css/Pair.h:67:  Should be indented on a separate line,
> with the colon or comma first on that line.  [whitespace/indent] [4]
> ERROR: Source/WebCore/css/Pair.h:68:  Should be indented on a separate line,
> with the colon or comma first on that line.  [whitespace/indent] [4]
> ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and
> either add and list tests, or explain why no new tests were possible. 
> [changelog/nonewtests] [5]
> ERROR: Source/JavaScriptCore/bytecode/StructureStubClearingWatchpoint.h:53: 
> Wrong number of spaces before statement. (expected: 12)  [whitespace/indent]
> [4]
> ERROR: Source/WebKit2/Scripts/webkit/MessageReceiver-expected.cpp:77: 
> _result is incorrectly named. Don't use underscores in your identifier
> names.  [readability/naming/underscores] [4]
> ERROR: Source/WebKit2/Scripts/webkit/MessageReceiver-expected.cpp:96: 
> _result is incorrectly named. Don't use underscores in your identifier
> names.  [readability/naming/underscores] [4]
> Total errors found: 20 in 1206 files
> 
> 
> If any of these errors are false positives, please file a bug against
> check-webkit-style.

Fixing most of these errors would require changing lines of code that didn't have WTF::move in them, obscuring blame information and making this patch harder to review. I plan to leave these as-is for now.
Comment 25 Brady Eidson 2016-01-01 21:36:29 PST
Comment on attachment 268064 [details]
V. Replace WTF::move with WTFMove

I'll trust that this is really just a global replace - I did not look at every line. :)
Comment 26 Andy Estes 2016-01-02 00:09:13 PST
Comment on attachment 268064 [details]
V. Replace WTF::move with WTFMove

Committed r194496: <http://trac.webkit.org/changeset/194496>
Comment 27 Csaba Osztrogonác 2016-01-02 01:07:13 PST
(In reply to comment #26)
> Comment on attachment 268064 [details]
> V. Replace WTF::move with WTFMove
> 
> Committed r194496: <http://trac.webkit.org/changeset/194496>

It broke the Apple Windows build.
Comment 28 Andy Estes 2016-01-02 01:07:59 PST
(In reply to comment #27)
> (In reply to comment #26)
> > Comment on attachment 268064 [details]
> > V. Replace WTF::move with WTFMove
> > 
> > Committed r194496: <http://trac.webkit.org/changeset/194496>
> 
> It broke the Apple Windows build.

I think it just needs a clean build.
Comment 29 Andy Estes 2016-01-02 02:18:55 PST
(In reply to comment #28)
> (In reply to comment #27)
> > (In reply to comment #26)
> > > Comment on attachment 268064 [details]
> > > V. Replace WTF::move with WTFMove
> > > 
> > > Committed r194496: <http://trac.webkit.org/changeset/194496>
> > 
> > It broke the Apple Windows build.
> 
> I think it just needs a clean build.

Windows is building now as of r194497.
Comment 30 Simon Fraser (smfr) 2016-01-03 09:38:31 PST
WinCairo build is broken:
https://build.webkit.org/builders/WinCairo%2064-Bit%20Release?numbuilds=100
Comment 31 Andy Estes 2016-01-03 11:01:36 PST
(In reply to comment #30)
> WinCairo build is broken:
> https://build.webkit.org/builders/WinCairo%2064-Bit%20Release?numbuilds=100

It's having the same problem as the Apple Windows build, where a stale copy of StdLibExtras.h is being used. I wonder if this is a bug in a CMake build file shared between these two platforms.

A clean build should work around this, but we should really fix the underlying bug.
Comment 32 Andy Estes 2016-01-03 22:58:50 PST
(In reply to comment #31)
> (In reply to comment #30)
> > WinCairo build is broken:
> > https://build.webkit.org/builders/WinCairo%2064-Bit%20Release?numbuilds=100
> 
> It's having the same problem as the Apple Windows build, where a stale copy
> of StdLibExtras.h is being used. I wonder if this is a bug in a CMake build
> file shared between these two platforms.
> 
> A clean build should work around this, but we should really fix the
> underlying bug.

I filed <https://bugs.webkit.org/show_bug.cgi?id=152681> about this. I don't know how to access the WinCairo buildbot, so I'm not sure how to force a clean build.
Comment 33 Anders Carlsson 2016-01-04 08:38:11 PST
You know, you could just have done:

namespace WTF {
using std::move;
}

which seems to trigger the warnings just fine.
Comment 34 Michael Catanzaro 2016-01-04 08:57:01 PST
(In reply to comment #33)
> You know, you could just have done:
> 
> namespace WTF {
> using std::move;
> }
> 
> which seems to trigger the warnings just fine.

I don't think I understand; surely then we lose the static_asserts that this patch attempted to save? What's the point of having WTF::move at all if it's the same as std::move?