Bug 97257

Summary: [chromium] Enable more clang warnings
Product: WebKit Reporter: Tony Chang <tony>
Component: New BugsAssignee: Tony Chang <tony>
Status: RESOLVED FIXED    
Severity: Normal CC: jamesr, thakis, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch none

Description Tony Chang 2012-09-20 13:21:04 PDT
[chromium] Enable more clang warnings
Comment 1 Tony Chang 2012-09-20 13:25:12 PDT
Created attachment 164970 [details]
Patch
Comment 2 Tony Chang 2012-09-20 13:25:55 PDT
Not sure if there's a better way for me to consolidate these.  Let me know if you have any ideas.
Comment 3 WebKit Review Bot 2012-09-20 16:38:01 PDT
Comment on attachment 164970 [details]
Patch

Clearing flags on attachment: 164970

Committed r129178: <http://trac.webkit.org/changeset/129178>
Comment 4 WebKit Review Bot 2012-09-20 16:38:04 PDT
All reviewed patches have been landed.  Closing bug.
Comment 5 Nico Weber 2012-09-20 19:15:13 PDT
Comment on attachment 164970 [details]
Patch

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

> Source/WebKit/chromium/WebKit.gyp:1337
> +        ['clang==1', {

There's already a block like this around line 739. Remove this again?

(Also, you're not setting -Wunused-parameter here -- I suppose that's intentional? --- ah, the changelog says it is. So just revert the changes to this file, or at least delete the duplicate block eralier in this file.)

> Source/WebKit/chromium/WebKitUnitTests.gyp:230
> +                'cflags': ['-Wglobal-constructors', '-Wunused-parameter'],

No need to warn about global constructors in tests. We don't care about those.

> Tools/DumpRenderTree/DumpRenderTree.gyp/DumpRenderTree.gyp:533
> +                # FIXME: Add -Wglobal-constructors after fixing existing bugs.

Not sure if we care about them in DRT either.
Comment 6 Tony Chang 2012-09-21 11:16:27 PDT
Thanks for looking, Nico!

(In reply to comment #5)
> (From update of attachment 164970 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=164970&action=review
> 
> > Source/WebKit/chromium/WebKit.gyp:1337
> > +        ['clang==1', {
> 
> There's already a block like this around line 739. Remove this again?

Good catch, I removed the earlier block here: http://trac.webkit.org/changeset/129242

> > Source/WebKit/chromium/WebKitUnitTests.gyp:230
> > +                'cflags': ['-Wglobal-constructors', '-Wunused-parameter'],
> 
> No need to warn about global constructors in tests. We don't care about those.

Yeah, I had to revert this part of the change last night.  The TEST/TEST_F macros create static initializers (I didn't see it because I was using the shared build).

> > Tools/DumpRenderTree/DumpRenderTree.gyp/DumpRenderTree.gyp:533
> > +                # FIXME: Add -Wglobal-constructors after fixing existing bugs.
> 
> Not sure if we care about them in DRT either.

I'm not sure it makes a big difference either, but we do launch DRT many times when running the layout tests, so it might have a tiny impact on bot cycle speed (although today the long pole is http tests, so maybe it doesn't matter).