Bug 97257 – [chromium] Enable more clang warnings

• Component: New Bugs
• Status: RESOLVED FIXED

[Tony Chang]

[chromium] Enable more clang warnings

[Tony Chang]

Created attachment 164970 [details]
Patch

[Tony Chang]

Not sure if there's a better way for me to consolidate these.  Let me know if you have any ideas.

[WebKit Review Bot]

Comment on attachment 164970 [details]
Patch

Clearing flags on attachment: 164970

Committed r129178: <http://trac.webkit.org/changeset/129178>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Nico Weber]

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.

[Tony Chang]

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