Bug 23102 – turn on unused parameter warnings in WebCore

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Darin Adler]

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.

[Darin Adler]

Created attachment 26414 [details]
step 1 -- covers the simplest cases

[Alexey Proskuryakov]

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.

[Darin Adler]

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

[Darin Adler]

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

[Darin Adler]

Since Subversion is down right now, I'll post my corrected patch so I can land it later.

[Darin Adler]

Created attachment 26433 [details]
step 1 ready to land after fixing what Alexey asked

[Darin Adler]

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.

[Darin Adler]

Comment on attachment 26414 [details]
step 1 -- covers the simplest cases

Clearing review flag since this was landed.

[Darin Adler]

http://trac.webkit.org/changeset/39601

[Darin Adler]

Created attachment 26435 [details]
more work in progress

[Darin Adler]

Created attachment 26608 [details]
step 2 -- covers ObjC files

[Darin Adler]

Comment on attachment 26435 [details]
more work in progress

This patch is now bug 23239.

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

[Darin Adler]

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

[Darin Adler]

Comment on attachment 26608 [details]
step 2 -- covers ObjC files

Clearing review flag since this was landed in <http://trac.webkit.org/changeset/39806>.

[Darin Adler]

Created attachment 26613 [details]
step 3 -- covers mostly arguments used only in assertions

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

[Darin Adler]

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

[Darin Adler]

Comment on attachment 26613 [details]
step 3 -- covers mostly arguments used only in assertions

Landed as http://trac.webkit.org/changeset/39810

[Darin Adler]

Created attachment 26620 [details]
step 4 -- removes unused arguments that were unnecessary

[Oliver Hunt]

Comment on attachment 26620 [details]
step 4 -- removes unused arguments that were unnecessary

r=me assuming all tests pass

[Darin Adler]

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.

[Darin Adler]

Created attachment 26626 [details]
patch

[Darin Adler]

http://trac.webkit.org/changeset/39880