Bug 164498

Summary: [Cocoa] Enable two clang warnings recommended by Xcode
Product: WebKit Reporter: Andy Estes <aestes>
Component: New BugsAssignee: Andy Estes <aestes>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, darin, dbates, keith_miller, mark.lam, mitz, msaboff, sbarati, simon.fraser, thorton
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch none

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.