Bug 23102 - turn on unused parameter warnings in WebCore
Summary: turn on unused parameter warnings in WebCore
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:
Blocks:
 
Reported: 2009-01-04 09:45 PST by Darin Adler
Modified: 2009-01-13 16:16 PST (History)
1 user (show)

See Also:


Attachments
step 1 -- covers the simplest cases (266.68 KB, patch)
2009-01-04 10:04 PST, Darin Adler
no flags Details | Formatted Diff | Diff
step 1 ready to land after fixing what Alexey asked (230.05 KB, patch)
2009-01-05 09:14 PST, Darin Adler
no flags Details | Formatted Diff | Diff
other parts of this, work in progress (74.44 KB, patch)
2009-01-05 09:17 PST, Darin Adler
no flags Details | Formatted Diff | Diff
more work in progress (11.92 KB, patch)
2009-01-05 10:29 PST, Darin Adler
no flags Details | Formatted Diff | Diff
step 2 -- covers ObjC files (22.35 KB, patch)
2009-01-11 11:25 PST, Darin Adler
no flags Details | Formatted Diff | Diff
step 3 -- covers mostly arguments used only in assertions (12.45 KB, patch)
2009-01-11 12:27 PST, Darin Adler
no flags Details | Formatted Diff | Diff
step 4 -- removes unused arguments that were unnecessary (69.83 KB, patch)
2009-01-11 18:23 PST, Darin Adler
no flags Details | Formatted Diff | Diff
patch (23.58 KB, patch)
2009-01-12 00:16 PST, Darin Adler
sam: 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-04 09:45:21 PST
We have unused parameter warnings turned on in JavaScriptCore, but off in WebCore. If we turn them on we will probably find a few mistakes, so it's worth the effort.
Comment 1 Darin Adler 2009-01-04 10:04:30 PST
Created attachment 26414 [details]
step 1 -- covers the simplest cases
Comment 2 Alexey Proskuryakov 2009-01-05 04:24:57 PST
Comment on attachment 26414 [details]
step 1 -- covers the simplest cases

r=me

Is there any good in leaving long lists of changed files in ChangeLog when there are no per-file comments? I never found that useful, and did actually find it a little harmful, as they tend to show up as false hits when searching for meaningful changes.

-    ASSERT(m_handle == handle);

I find this kind of assertions extremely helpful as documentation at list, and would prefer to have them kept. You could use ASSERT_UNUSED from my patch that is pending review now.

+void Frame::adjustPageHeight(float *newBottom, float oldTop, float oldBottom, float /*bottomLimit*/)

You could move a star here.

+void InitArenaPool(ArenaPool *pool, const char *, unsigned int size, unsigned int align)

And here.

-    virtual PassRefPtr<TransformOperation> blend(const TransformOperation* from, double progress, bool blendToIdentity = false)
+    virtual PassRefPtr<TransformOperation> blend(const TransformOperation*, double, bool)

I don't think it's good style to have default arguments on base class version of a function, but not on overrides - this makes it important which version to use at call sites, which is against the purpose of virtual functions.
Comment 3 Darin Adler 2009-01-05 07:25:30 PST
(In reply to comment #2)
> Is there any good in leaving long lists of changed files in ChangeLog when
> there are no per-file comments?

I think there is some value in listing all the functions changed when there is a substantive change to the functions even if you don't have time to comment on each change. But in this case where there's no substantive change, it's better to leave them out.

I'll do the rest of the things you suggest.
Comment 4 Darin Adler 2009-01-05 07:29:38 PST
(In reply to comment #2)
> -    virtual PassRefPtr<TransformOperation> blend(const TransformOperation*
> from, double progress, bool blendToIdentity = false)
> +    virtual PassRefPtr<TransformOperation> blend(const TransformOperation*,
> double, bool)
> 
> I don't think it's good style to have default arguments on base class version
> of a function, but not on overrides - this makes it important which version to
> use at call sites, which is against the purpose of virtual functions.

I'm not sure I agree with this one entirely. The compiler doesn't help us keep these consistent. And I think in many cases the subclass version of the function isn't intended for calling at all, which is why I mark it "private". I'd prefer to mark it even more private, "uncallable", if the language had a way to do that, making it clear you'd only call it through a base class pointer.

I find it irritating to try to keep all the subclass argument lists and defaults in sync with the base class.

That having been said, I'll take your recommendation.
Comment 5 Darin Adler 2009-01-05 09:14:04 PST
Since Subversion is down right now, I'll post my corrected patch so I can land it later.
Comment 6 Darin Adler 2009-01-05 09:14:41 PST
Created attachment 26433 [details]
step 1 ready to land after fixing what Alexey asked
Comment 7 Darin Adler 2009-01-05 09:17:26 PST
Created attachment 26434 [details]
other parts of this, work in progress

This part needs to be landed in smaller pieces. And the UNUSED_PARAM stuff can be largely replaced by use of Alexey's new ASSERT_UNUSED macro once he lands that.
Comment 8 Darin Adler 2009-01-05 09:21:31 PST
Comment on attachment 26414 [details]
step 1 -- covers the simplest cases

Clearing review flag since this was landed.
Comment 9 Darin Adler 2009-01-05 09:28:17 PST
http://trac.webkit.org/changeset/39601
Comment 10 Darin Adler 2009-01-05 10:29:42 PST
Created attachment 26435 [details]
more work in progress
Comment 11 Darin Adler 2009-01-11 11:25:06 PST
Created attachment 26608 [details]
step 2 -- covers ObjC files
Comment 12 Darin Adler 2009-01-11 11:26:24 PST
Comment on attachment 26435 [details]
more work in progress

This patch is now bug 23239.
Comment 13 mitz 2009-01-11 11:30:52 PST
Comment on attachment 26608 [details]
step 2 -- covers ObjC files

> +#import <JavaScriptCore/UnusedParam.h>

Everywhere else you used <wtf/UnusedParam.h>. Why not here?

r=me
Comment 14 Darin Adler 2009-01-11 11:48:58 PST
(In reply to comment #13)
> (From update of attachment 26608 [details] [review])
> > +#import <JavaScriptCore/UnusedParam.h>
> 
> Everywhere else you used <wtf/UnusedParam.h>. Why not here?

I'm so glad you asked that. There's no reason. I noticed it myself in the patch.

I'm conflicted about how to handle includes from Mac-platform-specific files. Whether to use framework-style includes <JavaScriptCore/Vector.h> or whether to use the simulated Unix-library-style includes <wtf/Vector.h>. Someone should make a policy decision so I don't have to think so much.

I'll use wtf here.
Comment 15 Darin Adler 2009-01-11 12:12:09 PST
Comment on attachment 26608 [details]
step 2 -- covers ObjC files

Clearing review flag since this was landed in <http://trac.webkit.org/changeset/39806>.
Comment 16 Darin Adler 2009-01-11 12:27:32 PST
Created attachment 26613 [details]
step 3 -- covers mostly arguments used only in assertions
Comment 17 mitz 2009-01-11 12:42:16 PST
Comment on attachment 26613 [details]
step 3 -- covers mostly arguments used only in assertions

>      setDatabaseVersion.begin();
>  
>      char userVersionSQL[32];
> -    int numBytes = snprintf(userVersionSQL, sizeof(userVersionSQL), "PRAGMA user_version=%d", schemaVersion);
> -    ASSERT_UNUSED(numBytes, static_cast<int>(sizeof(userVersionSQL)) >= numBytes);
> +    int unusedNumBytes = snprintf(userVersionSQL, sizeof(userVersionSQL), "PRAGMA user_version=%d", schemaVersion);
> +    ASSERT_UNUSED(unusedNumBytes, static_cast<int>(sizeof(userVersionSQL)) >= unusedNumBytes);

Is there a reason to even call sprintf() if assertions are disabled? If not, then both lines should be #if-ed to compile only when assertions are enabled.

r=me
Comment 18 Darin Adler 2009-01-11 13:26:10 PST
(In reply to comment #17)
> >      char userVersionSQL[32];
> > -    int numBytes = snprintf(userVersionSQL, sizeof(userVersionSQL), "PRAGMA user_version=%d", schemaVersion);
> > -    ASSERT_UNUSED(numBytes, static_cast<int>(sizeof(userVersionSQL)) >= numBytes);
> > +    int unusedNumBytes = snprintf(userVersionSQL, sizeof(userVersionSQL), "PRAGMA user_version=%d", schemaVersion);
> > +    ASSERT_UNUSED(unusedNumBytes, static_cast<int>(sizeof(userVersionSQL)) >= unusedNumBytes);
> 
> Is there a reason to even call sprintf() if assertions are disabled?

Yes, snprintf if needed in any case, to construct the SQL command; it's side effect is the important part. The assertion-only part is asserting that the return value from snprintf is what's expected.
Comment 19 Darin Adler 2009-01-11 14:01:48 PST
Comment on attachment 26613 [details]
step 3 -- covers mostly arguments used only in assertions

Landed as http://trac.webkit.org/changeset/39810
Comment 20 Darin Adler 2009-01-11 18:23:39 PST
Created attachment 26620 [details]
step 4 -- removes unused arguments that were unnecessary
Comment 21 Oliver Hunt 2009-01-11 21:42:51 PST
Comment on attachment 26620 [details]
step 4 -- removes unused arguments that were unnecessary

r=me assuming all tests pass
Comment 22 Darin Adler 2009-01-11 23:45:35 PST
Comment on attachment 26620 [details]
step 4 -- removes unused arguments that were unnecessary

Landed as http://trac.webkit.org/changeset/39818 so clearing the review flag.
Comment 23 Darin Adler 2009-01-12 00:16:58 PST
Created attachment 26626 [details]
patch
Comment 24 Darin Adler 2009-01-13 16:16:50 PST
http://trac.webkit.org/changeset/39880