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.
Created attachment 26414 [details] step 1 -- covers the simplest cases
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.
(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.
(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.
Since Subversion is down right now, I'll post my corrected patch so I can land it later.
Created attachment 26433 [details] step 1 ready to land after fixing what Alexey asked
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 on attachment 26414 [details] step 1 -- covers the simplest cases Clearing review flag since this was landed.
http://trac.webkit.org/changeset/39601
Created attachment 26435 [details] more work in progress
Created attachment 26608 [details] step 2 -- covers ObjC files
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
(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 on attachment 26608 [details] step 2 -- covers ObjC files Clearing review flag since this was landed in <http://trac.webkit.org/changeset/39806>.
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
(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 on attachment 26613 [details] step 3 -- covers mostly arguments used only in assertions Landed as http://trac.webkit.org/changeset/39810
Created attachment 26620 [details] step 4 -- removes unused arguments that were unnecessary
Comment on attachment 26620 [details] step 4 -- removes unused arguments that were unnecessary r=me assuming all tests pass
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.
Created attachment 26626 [details] patch
http://trac.webkit.org/changeset/39880