Bug 164498

Summary:

[Cocoa] Enable two clang warnings recommended by Xcode

Product:

WebKit

Reporter:

Andy Estes <aestes>

Component:

New Bugs

Assignee:

Andy Estes <aestes>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

commit-queue, darin, dbates, keith_miller, mark.lam, mitz, msaboff, saam, simon.fraser, thorton

Priority:

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.