Bug 152601 – Use of WTF::move prevents clang's move diagnostics from warning about several classes of mistakes

• Component: Web Template Framework
• Status: RESOLVED FIXED

[Andy Estes]

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.

[Andy Estes]

Created attachment 268014 [details]
I. Add std::move<WTF::CheckMoveParameter> and WTF_MOVE

[Andy Estes]

Created attachment 268016 [details]
I. Add std::move<WTF::CheckMoveParameter> and WTF_MOVE

[WebKit Commit Bot]

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>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Andy Estes]

Reopening to add part 2.

[Daniel Bates]

(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 :/

[Daniel Bates]

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>.

[Andy Estes]

(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().

[Andy Estes]

(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.

[Andy Estes]

Created attachment 268050 [details]
II. Fix warnings uncovered by migrating to WTF_MOVE

[Andy Estes]

Created attachment 268051 [details]
III. Rename WTF_MOVE to WTFMove

[Andy Estes]

Created attachment 268052 [details]
IV. Update the style checker to advise using WTFMove() instead of WTF::move()

[WebKit Commit Bot]

Comment on attachment 268051 [details]
III. Rename WTF_MOVE to WTFMove

Clearing flags on attachment: 268051

Committed r194469: <http://trac.webkit.org/changeset/194469>

[WebKit Commit Bot]

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>

[WebKit Commit Bot]

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>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Andy Estes]

Reopening again to remove WTF::move and do the WTF::move-to-WTFMove rename.

[Ryan Haddad]

(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>

[David Kilzer (:ddkilzer)]

(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>

[David Kilzer (:ddkilzer)]

Oops, Andy had one more step per Comment #17.

[Andy Estes]

(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!

[Andy Estes]

Created attachment 268064 [details]
V. Replace WTF::move with WTFMove

[WebKit Commit Bot]

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.

[Andy Estes]

(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.

[Brady Eidson]

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. :)

[Andy Estes]

Comment on attachment 268064 [details]
V. Replace WTF::move with WTFMove

Committed r194496: <http://trac.webkit.org/changeset/194496>

[Csaba Osztrogonác]

(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.

[Andy Estes]

(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.

[Andy Estes]

(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.

[Simon Fraser (smfr)]

WinCairo build is broken:
https://build.webkit.org/builders/WinCairo%2064-Bit%20Release?numbuilds=100

[Andy Estes]

(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.

[Andy Estes]

(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.

[Anders Carlsson]

You know, you could just have done:

namespace WTF {
using std::move;
}

which seems to trigger the warnings just fine.

[Michael Catanzaro]

(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?