search

Quuxplusone / LLVMBugzillaTest

0 stars 0 forks source link

clang should warn if a class and its subclass have a method with the same signature but no "virtual" in base class #10532

Open Quuxplusone opened 13 years ago

Quuxplusone commented 13 years ago
Bugzilla Link PR10234
Status REOPENED
Importance P normal
Reported by Nico Weber (nicolasweber@gmx.de)
Reported on 2011-06-30 18:25:56 -0700
Last modified on 2012-03-01 09:02:22 -0800
Version trunk
Hardware PC All
CC dgregor@apple.com, geek4civic@gmail.com, hans@chromium.org, llvm-bugs@lists.llvm.org
Fixed by commit(s)
Attachments missing_virtual_first_stab.patch (1678 bytes, text/plain)
Blocks
Blocked by
See also 
Consider

class A {
public:
  void f() {}
};

class B : public A {
public:
  void f() {}
};

It is likely that B::m is supposed to be an override of A::m, but "virtual" is
missing. We should add this warning and check how noisy it is. If it's not
noisy, it should be added to the -Wall group.
Quuxplusone commented 13 years ago

If the naive check ends up being noisy, we could try only warning if the method in question ends up being called through a base class pointer.

Quuxplusone commented 13 years ago

Attached missing_virtual_first_stab.patch (1678 bytes, text/plain): First stab at warning

Quuxplusone commented 13 years ago

Can you paste a few example versions for RefCounted and IPC::Message?

You understood my suggestion correctly. Can you think of other heuristics that could make this work better? Here's another bug this would've caught: http://codereview.chromium.org/7468010/

Quuxplusone commented 13 years ago 
(In reply to comment #3)
> Can you paste a few example versions for RefCounted and IPC::Message?
>
> You understood my suggestion correctly. Can you think of other heuristics that
> could make this work better? Here's another bug this would've caught:
> http://codereview.chromium.org/7468010/

Some examples using the patch I attached before:

/base/memory/ref_counted.h:87:3: error: 'base::RefCounted::~RefCounted<T>'
would override 'base::subtle::RefCountedBase::~RefCountedBase' if it
      were virtual [-Werror]

./base/memory/ref_counted.h:89:8: error: 'base::RefCounted::AddRef' would
override 'base::subtle::RefCountedBase::AddRef' if it were virtual
      [-Werror]

./content/common/devtools_messages.h:64:1: error:
'DevToolsClientMsg_DebuggerOutput::Log' would override 'IPC::Message::Log' if
it were virtual

third_party/WebKit/Source/JavaScriptCore/wtf/Vector.h:365:9: warning:
'WTF::VectorBuffer<char, 0>::~VectorBuffer' would override
      'WTF::VectorBufferBase<char>::~VectorBufferBase' if it were virtual
Quuxplusone commented 12 years ago

Looks like this won't fly, then.

Quuxplusone commented 12 years ago

How much better does this get when you filter out destructors?

(see http://crbug.com/115925 for another bug like this :-/)