WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
23102
turn on unused parameter warnings in WebCore
https://bugs.webkit.org/show_bug.cgi?id=23102
Summary
turn on unused parameter warnings in WebCore
Darin Adler
Reported
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.
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
Show Obsolete
(7)
View All
Add attachment
proposed patch, testcase, etc.
Darin Adler
Comment 1
2009-01-04 10:04:30 PST
Created
attachment 26414
[details]
step 1 -- covers the simplest cases
Alexey Proskuryakov
Comment 2
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.
Darin Adler
Comment 3
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.
Darin Adler
Comment 4
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.
Darin Adler
Comment 5
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.
Darin Adler
Comment 6
2009-01-05 09:14:41 PST
Created
attachment 26433
[details]
step 1 ready to land after fixing what Alexey asked
Darin Adler
Comment 7
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.
Darin Adler
Comment 8
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.
Darin Adler
Comment 9
2009-01-05 09:28:17 PST
http://trac.webkit.org/changeset/39601
Darin Adler
Comment 10
2009-01-05 10:29:42 PST
Created
attachment 26435
[details]
more work in progress
Darin Adler
Comment 11
2009-01-11 11:25:06 PST
Created
attachment 26608
[details]
step 2 -- covers ObjC files
Darin Adler
Comment 12
2009-01-11 11:26:24 PST
Comment on
attachment 26435
[details]
more work in progress This patch is now
bug 23239
.
mitz
Comment 13
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
Darin Adler
Comment 14
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.
Darin Adler
Comment 15
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
>.
Darin Adler
Comment 16
2009-01-11 12:27:32 PST
Created
attachment 26613
[details]
step 3 -- covers mostly arguments used only in assertions
mitz
Comment 17
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
Darin Adler
Comment 18
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.
Darin Adler
Comment 19
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
Darin Adler
Comment 20
2009-01-11 18:23:39 PST
Created
attachment 26620
[details]
step 4 -- removes unused arguments that were unnecessary
Oliver Hunt
Comment 21
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
Darin Adler
Comment 22
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.
Darin Adler
Comment 23
2009-01-12 00:16:58 PST
Created
attachment 26626
[details]
patch
Darin Adler
Comment 24
2009-01-13 16:16:50 PST
http://trac.webkit.org/changeset/39880
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug