Bug 23352 - Turn on more compiler warnings in the Mac build
Summary: Turn on more compiler warnings in the Mac build
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh OS X 10.5
: P2 Normal
Assignee: Darin Adler
URL:
Keywords:
Depends on: 23120
Blocks:
  Show dependency treegraph
 
Reported: 2009-01-15 11:39 PST by Darin Adler
Modified: 2009-01-25 19:02 PST (History)
3 users (show)

See Also:


Attachments
work in progress (18.01 KB, patch)
2009-01-16 09:11 PST, Darin Adler
no flags Details | Formatted Diff | Diff
step 1 -- covers the simplest cases (9.16 KB, patch)
2009-01-19 20:49 PST, Darin Adler
no flags Details | Formatted Diff | Diff
step 2 -- slightly more complex fixes (8.13 KB, patch)
2009-01-21 00:05 PST, Darin Adler
no flags Details | Formatted Diff | Diff
step 3 -- noreturn (3.52 KB, patch)
2009-01-24 13:49 PST, Darin Adler
no flags Details | Formatted Diff | Diff
step 4 -- last couple of warnings (4.17 KB, patch)
2009-01-24 13:49 PST, Darin Adler
no flags Details | Formatted Diff | Diff
last step -- turn the warnings on (4.07 KB, patch)
2009-01-25 13:45 PST, Darin Adler
mrowe: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Darin Adler 2009-01-15 11:39:59 PST
I played around with some more compiler warnings we can turn on the Mac build. I'll attach my patch.
Comment 1 Darin Adler 2009-01-16 09:11:10 PST
Created attachment 26796 [details]
work in progress
Comment 2 Darin Adler 2009-01-16 09:58:53 PST
All I have to do is:

    1) Check that all these warnings and warning options work OK in the older version of the compiler we still use to build on Tiger.

    2) Check that none of these warning switches is redundant. Some of these warnings might already be on as a side effect of -Wall or -W or one of the Xcode warning options.

And then this could probably be landed any time. The elephant in the room, though, is -Woverloaded-virtual.

Also, we could use -Wextra instead of -W if we check that it's supported in all the versions of gcc we need to support.
Comment 3 Darin Adler 2009-01-19 20:49:04 PST
Created attachment 26854 [details]
step 1 -- covers the simplest cases
Comment 4 Mark Rowe (bdash) 2009-01-19 21:18:29 PST
One thing I noticed in the WIP patch is that you added -Wnewline-eof to WARNING_CFLAGS_BASE.  It'd be preferable to just set GCC_WARN_ABOUT_MISSING_NEWLINE = YES instead.
Comment 5 Mark Rowe (bdash) 2009-01-19 21:20:33 PST
Comment on attachment 26854 [details]
step 1 -- covers the simplest cases

r=me
Comment 6 Darin Adler 2009-01-20 10:26:06 PST
Comment on attachment 26854 [details]
step 1 -- covers the simplest cases

Landed this http://trac.webkit.org/changeset/40059 so clearing the review flag.
Comment 7 Darin Adler 2009-01-21 00:05:14 PST
Created attachment 26889 [details]
step 2 -- slightly more complex fixes
Comment 8 Anders Carlsson 2009-01-23 18:10:57 PST
Comment on attachment 26889 [details]
step 2 -- slightly more complex fixes

r=me
Comment 9 Darin Adler 2009-01-23 18:40:28 PST
Comment on attachment 26889 [details]
step 2 -- slightly more complex fixes

Clearing review flag because I landed this as <http://trac.webkit.org/changeset/40207>.
Comment 10 Darin Adler 2009-01-24 13:49:27 PST
Created attachment 26998 [details]
step 3 -- noreturn
Comment 11 Darin Adler 2009-01-24 13:49:52 PST
Created attachment 26999 [details]
step 4 -- last couple of warnings
Comment 12 Darin Adler 2009-01-25 12:25:56 PST
Comment on attachment 26998 [details]
step 3 -- noreturn

Landed as trac.webkit.org/changeset/40230 so clearing review flag.
Comment 13 Darin Adler 2009-01-25 12:28:26 PST
Comment on attachment 26999 [details]
step 4 -- last couple of warnings

Landed as trac.webkit.org/changeset/40231 so clearing review flag.
Comment 14 Darin Adler 2009-01-25 12:39:42 PST
(In reply to comment #4)
> One thing I noticed in the WIP patch is that you added -Wnewline-eof to
> WARNING_CFLAGS_BASE.  It'd be preferable to just set
> GCC_WARN_ABOUT_MISSING_NEWLINE = YES instead.

GCC_WARN_ABOUT_MISSING_NEWLINE = YES is already set, but for some reason -Wnewline-eof was *not* being passed in to the compiler.
Comment 15 Darin Adler 2009-01-25 12:55:35 PST
(In reply to comment #14)
> GCC_WARN_ABOUT_MISSING_NEWLINE = YES is already set, but for some reason
> -Wnewline-eof was *not* being passed in to the compiler.

My mistake. I think this was already set and already working.
Comment 16 Darin Adler 2009-01-25 13:45:15 PST
Created attachment 27026 [details]
last step -- turn the warnings on
Comment 17 Mark Rowe (bdash) 2009-01-25 18:55:05 PST
Comment on attachment 27026 [details]
last step -- turn the warnings on

r=me.  Are we going to turn these warnings on in WebKit as well?
Comment 18 Darin Adler 2009-01-25 18:58:40 PST
(In reply to comment #17)
> Are we going to turn these warnings on in WebKit as well?

Well, WebKit is not yet ready for uninitialized-parameter -- since it's Objective-C, there are a *lot* of those. But I did a diff on JavaScriptCore vs. WebCore and WebCore vs. WebKit to see if there were any interesting differences. I found a few that I want to talk to you about.
Comment 19 Darin Adler 2009-01-25 19:02:33 PST
http://trac.webkit.org/changeset/40237