Bug 23120 - Turn on overloaded virtual functions warning in WebCore
Summary: Turn on overloaded virtual functions warning in WebCore
Status: RESOLVED WONTFIX
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: Nobody
URL:
Keywords:
Depends on:
Blocks: 23352
  Show dependency treegraph
 
Reported: 2009-01-05 11:44 PST by Simon Fraser (smfr)
Modified: 2011-04-17 20:57 PDT (History)
4 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Fraser (smfr) 2009-01-05 11:44:06 PST
-Woverloaded-virtual should help us avoid bugs like bug 23082.
Comment 1 Simon Fraser (smfr) 2009-01-05 11:53:33 PST
diff --git a/WebCore/Configurations/Base.xcconfig b/WebCore/Configurations/Base.xcconfig
index 803911a..9476735 100644
--- a/WebCore/Configurations/Base.xcconfig
+++ b/WebCore/Configurations/Base.xcconfig
@@ -16,6 +16,7 @@ GCC_WARN_ABOUT_DEPRECATED_FUNCTIONS = NO;
 GCC_WARN_ABOUT_MISSING_NEWLINE = YES;
 GCC_WARN_ABOUT_MISSING_PROTOTYPES = YES;
 GCC_WARN_NON_VIRTUAL_DESTRUCTOR = YES;
+GCC_WARN_HIDDEN_VIRTUAL_FUNCTIONS = YES;
 LINKER_DISPLAYS_MANGLED_NAMES = YES;
 PREBINDING = NO;
 VALID_ARCHS = i386 ppc x86_64 ppc64;
Comment 2 Darin Adler 2009-01-15 11:37:29 PST
The first thing we'd need to do would be to change names in JavaScriptCore for the overloads where functions take either an Identifier or an integer. That probably affects 10 or so functions in JavaScriptCore and would be a simple enough change.

The naming I suggest is to use the word "index" in the names. So for "hasProperty" you could make the version that takes an integer be named "hasIndexProperty".

In fact, I think turning this warning on for JavaScriptCore is a separate step and could almost deserve a separate bug.
Comment 3 Simon Fraser (smfr) 2009-01-15 11:41:19 PST
The first issue I hit in WebCore was:
    virtual void imageChanged(CachedImage*, const IntRect* = 0);
    virtual void imageChanged(WrappedImagePtr, const IntRect* = 0) { }
Comment 4 Darin Adler 2009-01-15 11:43:18 PST
(In reply to comment #3)
> The first issue I hit in WebCore was:
>     virtual void imageChanged(CachedImage*, const IntRect* = 0);
>     virtual void imageChanged(WrappedImagePtr, const IntRect* = 0) { }

Seems another case where we can do a name change, but probably one that affects a lot fewer files.
Comment 5 Cameron Zwarich (cpst) 2011-02-13 16:08:31 PST
I fixed the JSC issues for Clang (which enables this by default) in bug 53760. If anything, I would expect to Clang to be stricter than GCC. Here's a bug to turn the warning on in GCC:

Bug 54369 - Turn on -Woverloaded-virtual for GCC builds of JSC
Comment 6 Cameron Zwarich (cpst) 2011-02-13 16:22:52 PST
Actually, as I point out in bug 54369, the GCC warning has way too many false positives in Apple's GCC 4.2, and I don't think we could ever turn it on for JSC. The Clang warning is much saner.
Comment 7 Cameron Zwarich (cpst) 2011-04-17 20:57:48 PDT
I'm just going to close this, because the GCC warning is broken and the sane version is on by default in Clang.