Bug 164498 - [Cocoa] Enable two clang warnings recommended by Xcode
Summary: [Cocoa] Enable two clang warnings recommended by Xcode
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Andy Estes
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-11-07 17:31 PST by Andy Estes
Modified: 2016-11-29 13:40 PST (History)
10 users (show)

See Also:

Attachments
Patch (11.80 KB, patch)
2016-11-29 11:44 PST, Andy Estes 		no flags Details | Formatted Diff | Diff
View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Andy Estes 2016-11-07 17:31:43 PST 
[Cocoa] Enable two clang warnings recommended by Xcode
Comment 1 Andy Estes 2016-11-29 11:44:26 PST 
Created attachment 295614 [details]
Patch
Comment 2 Mark Lam 2016-11-29 13:10:57 PST 
Comment on attachment 295614 [details]
Patch

r=me
Comment 3 Daniel Bates 2016-11-29 13:14:55 PST 
Comment on attachment 295614 [details]
Patch

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

> Source/JavaScriptCore/Configurations/Base.xcconfig:40
> +CLANG_WARN_SUSPICIOUS_MOVE = YES;

Does this mean that we can get rid of our std::move() overload/WTFMove()?
Comment 4 Andy Estes 2016-11-29 13:31:20 PST 
(In reply to comment #3)
> Comment on attachment 295614 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=295614&action=review
> 
> > Source/JavaScriptCore/Configurations/Base.xcconfig:40
> > +CLANG_WARN_SUSPICIOUS_MOVE = YES;
> 
> Does this mean that we can get rid of our std::move() overload/WTFMove()?

No, we still need WTFMove() for -Wmove to function properly; clang's diagnostic only analyzes single-parameter functions named "move" in the std namespace.
Comment 5 WebKit Commit Bot 2016-11-29 13:37:10 PST 
Comment on attachment 295614 [details]
Patch

Clearing flags on attachment: 295614

Committed r209088: <http://trac.webkit.org/changeset/209088>
Comment 6 WebKit Commit Bot 2016-11-29 13:37:15 PST 
All reviewed patches have been landed.  Closing bug.
Comment 7 Andy Estes 2016-11-29 13:40:30 PST 
(In reply to comment #4)
> (In reply to comment #3)
> > Comment on attachment 295614 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=295614&action=review
> > 
> > > Source/JavaScriptCore/Configurations/Base.xcconfig:40
> > > +CLANG_WARN_SUSPICIOUS_MOVE = YES;
> > 
> > Does this mean that we can get rid of our std::move() overload/WTFMove()?
> 
> No, we still need WTFMove() for -Wmove to function properly; clang's
> diagnostic only analyzes single-parameter functions named "move" in the std
> namespace.

Actually we didn't need to enable CLANG_WARN_SUSPICIOUS_MOVE, since -Wmove is included in -Wall. Looks like Xcode isn't smart enough to figure that out.

It doesn't hurt to be explicit though, in case that changes.