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 :-( - 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. On Thu, 2004-06-24 at 06:40, Vladimir Prus 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. > > 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 :-( > > - Volodya > > > _______________________________________________ > LLVM Developers mailing list > LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu > http://mail.cs.uiuc.edu/mailman/listinfo/llvmdev-------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 189 bytes Desc: This is a digitally signed message part URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20040624/2648653a/attachment.sig>
On Thu, 24 Jun 2004, Vladimir Prus 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 :) I definitely think that it would be a good idea to add this -W flag :)> 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 :-(That is not a big deal necessarily. Is this the only case that currently causes spurious warnings? -Chris -- http://llvm.cs.uiuc.edu/ http://www.nondot.org/~sabre/Projects/
On Thu, 24 Jun 2004, Reid Spencer wrote:> 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> On Thu, 2004-06-24 at 06:40, Vladimir Prus 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. > > > > 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 :-( > > > > - Volodya > > > > > > _______________________________________________ > > LLVM Developers mailing list > > LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu > > http://mail.cs.uiuc.edu/mailman/listinfo/llvmdev >-Chris -- http://llvm.cs.uiuc.edu/ http://www.nondot.org/~sabre/Projects/
Chris Lattner wrote:> On Thu, 24 Jun 2004, Vladimir Prus 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 :)Interesting!> I definitely think that it would be a good idea to add this -W flag :)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 > >k>*, 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 :-( > > 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