-Woverloaded-virtual

I've just had some fun, because I wanted to override
FunctionPass::addAnalysisUsage, but forgot "const" after the method name --
so instead of overriding I've just created a new unrelated method.

After spending some time on this, I've decided to add -Woverloaded-virtual
option to compiler to catch such cases. However, it also gives some warnings
on LLVM code:

../../../include/llvm/Pass.h:264: warning: `virtual bool
   llvm::FunctionPass::run(llvm::Module&)' was hidden
../../../include/llvm/Pass.h:326: warning: by `bool
   llvm::BasicBlockPass::run(llvm::BasicBlock&)'
../../../include/llvm/Pass.h:275: warning: `virtual void
   llvm::FunctionPass::addToPassManager(llvm::PassManagerT<llvm::Module>*,
   llvm::AnalysisUsage&)' was hidden
../../../include/llvm/Pass.h:332: warning: by `virtual void
   llvm::BasicBlockPass::addToPassManager(llvm::PassManagerT<llvm::BasicBlock>*,
   llvm::AnalysisUsage&)'

The problem is that "run" method is virtual in Path (with Module& as
argument), but another version (non-virtual), which takes BasicBlock& is
defined in BasicBlockPath.

Do you think this warning is worth fixing? One possible approach is to rename
virtual run(Module&) to runOnModule, but that would require updating all
derived classes :frowning:

- Volodya

I've long been an advocate of using -Woverloaded-virtual. The thing
with this option is that it can REALLY help catch some nasty inheritance
bugs. I'm running into these as I'm designing the bytecode analyzer
interface. I make a change to the interface, forget to change a
subclass, and bingo, that method doesn't get called any more and the
compiler doesn't warn me about it.

I asked Chris about this months ago but there was little interest in
changing it. Perhaps we need to file a bug to take care of the warnings
it produces so that LLVm is overloaded virtual clean, then we can turn
on checking for overloaded virtuals.

Reid.

I've just had some fun, because I wanted to override
FunctionPass::addAnalysisUsage, but forgot "const" after the method name --
so instead of overriding I've just created a new unrelated method.

Ya know, I think that everyone has done that at least once (myself
included)... on THAT VERY METHOD. grr :slight_smile:

I definitely think that it would be a good idea to add this -W flag :slight_smile:

After spending some time on this, I've decided to add
-Woverloaded-virtual option to compiler to catch such cases. However, it
also gives some warnings on LLVM code:

../../../include/llvm/Pass.h:264: warning: `virtual bool
   llvm::FunctionPass::run(llvm::Module&)' was hidden
../../../include/llvm/Pass.h:326: warning: by `bool
   llvm::BasicBlockPass::run(llvm::BasicBlock&)'
../../../include/llvm/Pass.h:275: warning: `virtual void
   llvm::FunctionPass::addToPassManager(llvm::PassManagerT<llvm::Module>*,
   llvm::AnalysisUsage&)' was hidden
../../../include/llvm/Pass.h:332: warning: by `virtual void
   llvm::BasicBlockPass::addToPassManager(llvm::PassManagerT<llvm::BasicBlock>*,
   llvm::AnalysisUsage&)'

The problem is that "run" method is virtual in Path (with Module& as
argument), but another version (non-virtual), which takes BasicBlock& is
defined in BasicBlockPath.

Is that the only problem case?

Do you think this warning is worth fixing? One possible approach is to
rename virtual run(Module&) to runOnModule, but that would require
updating all derived classes :frowning:

That is not a big deal necessarily. Is this the only case that currently
causes spurious warnings?

-Chris

I asked Chris about this months ago but there was little interest in
changing it.

I thought we were talking about -Weffc++ at that time?

Perhaps we need to file a bug to take care of the warnings it produces
so that LLVm is overloaded virtual clean, then we can turn on checking
for overloaded virtuals.

This sounds like a great way to do it.

-Chris

> I asked Chris about this months ago but there was little interest in
> changing it.

I thought we were talking about -Weffc++ at that time?

No, that was recently. I was talking about months ago (January this
year?). it was a small conversation.

> Perhaps we need to file a bug to take care of the warnings it produces
> so that LLVm is overloaded virtual clean, then we can turn on checking
> for overloaded virtuals.

This sounds like a great way to do it.

yeah, I'll file the bug.

Chris Lattner wrote:

> I've just had some fun, because I wanted to override
> FunctionPass::addAnalysisUsage, but forgot "const" after the method name
> -- so instead of overriding I've just created a new unrelated method.

Ya know, I think that everyone has done that at least once (myself
included)... on THAT VERY METHOD. grr :slight_smile:

Interesting!

I definitely think that it would be a good idea to add this -W flag :slight_smile:

Good.

> After spending some time on this, I've decided to add
> -Woverloaded-virtual option to compiler to catch such cases. However, it
> also gives some warnings on LLVM code:
>
> ../../../include/llvm/Pass.h:264: warning: `virtual bool
> llvm::FunctionPass::run(llvm::Module&)' was hidden
> ../../../include/llvm/Pass.h:326: warning: by `bool
> llvm::BasicBlockPass::run(llvm::BasicBlock&)'
> ../../../include/llvm/Pass.h:275: warning: `virtual void
>
> llvm::FunctionPass::addToPassManager(llvm::PassManagerT<llvm::Module>*,
> llvm::AnalysisUsage&)' was hidden
> ../../../include/llvm/Pass.h:332: warning: by `virtual void
>
> llvm::BasicBlockPass::addToPassManager(llvm::PassManagerT<llvm::BasicBloc
>>*, llvm::AnalysisUsage&)'
>
> The problem is that "run" method is virtual in Path (with Module& as
> argument), but another version (non-virtual), which takes BasicBlock& is
> defined in BasicBlockPath.

Is that the only problem case?

No, the Pass::print is sometimes hidden as well. Maybe there are other issues,
but the above ones happen on every file, so it's hard to see anything more.
The build log is at

   http://zigzag.cs.msu.su:7813/bjam.log.bz2

if you want to take a look.

> Do you think this warning is worth fixing? One possible approach is to
> rename virtual run(Module&) to runOnModule, but that would require
> updating all derived classes :frowning:

That is not a big deal necessarily. Is this the only case that currently
causes spurious warnings?

That and Pass::print. Any other, if any, are not so severe.

- Volodya