From d9e627039245dc42f003a7cf75642f619a621513 Mon Sep 17 00:00:00 2001 From: Noel Grandin Date: Tue, 17 Nov 2015 11:16:07 +0200 Subject: loplugin:unnecessaryvirtual update the plugin with lessons learned from the mergeclasses plugin and re-run it Change-Id: I9d622eb3d05fceaf8fa764c533c8fa5dfb4c7711 Reviewed-on: https://gerrit.libreoffice.org/20015 Tested-by: Jenkins Reviewed-by: Noel Grandin --- compilerplugins/clang/store/unnecessaryvirtual.cxx | 261 +++++++++------------ compilerplugins/clang/store/unnecessaryvirtual.py | 57 +---- 2 files changed, 118 insertions(+), 200 deletions(-) (limited to 'compilerplugins') diff --git a/compilerplugins/clang/store/unnecessaryvirtual.cxx b/compilerplugins/clang/store/unnecessaryvirtual.cxx index 53688cb03a46..5a6d63384750 100644 --- a/compilerplugins/clang/store/unnecessaryvirtual.cxx +++ b/compilerplugins/clang/store/unnecessaryvirtual.cxx @@ -13,6 +13,7 @@ #include #include "plugin.hxx" #include "compat.hxx" +#include /** Dump a list of virtual methods and a list of methods overriding virtual methods. @@ -20,31 +21,51 @@ Then we will post-process the 2 lists and find the set of virtual methods which The process goes something like this: $ make check - $ make FORCE_COMPILE_ALL=1 COMPILER_PLUGIN_TOOL='unnecessaryvirtual' check > log.txt - $ ./compilerplugins/clang/unnecessaryvirtual.py log.txt > result.txt + $ make FORCE_COMPILE_ALL=1 COMPILER_PLUGIN_TOOL='unnecessaryvirtual' check + $ ./compilerplugins/clang/unnecessaryvirtual.py unnecessaryvirtual.log > result.txt $ for dir in *; do make FORCE_COMPILE_ALL=1 UPDATE_FILES=$dir COMPILER_PLUGIN_TOOL='removevirtuals' $dir; done Note that the actual process may involve a fair amount of undoing, hand editing, and general messing around to get it to work :-) -TODO function template instantiations are not handled TODO some boost bind stuff appears to confuse it, notably in the xmloff module +TODO does not find destructors that don't need to be virtual */ namespace { +// try to limit the voluminous output a little +static std::set definitionSet; +static std::set overridingSet; + class UnnecessaryVirtual: public RecursiveASTVisitor, public loplugin::Plugin { public: explicit UnnecessaryVirtual(InstantiationData const & data): Plugin(data) {} - virtual void run() override { TraverseDecl(compiler.getASTContext().getTranslationUnitDecl()); } + virtual void run() override + { + TraverseDecl(compiler.getASTContext().getTranslationUnitDecl()); + + // dump all our output in one write call - this is to try and limit IO "crosstalk" between multiple processes + // writing to the same logfile + std::string output; + for (const std::string & s : definitionSet) + output += "definition:\t" + s + "\n"; + for (const std::string & s : overridingSet) + output += "overriding:\t" + s + "\n"; + ofstream myfile; + myfile.open( SRCDIR "/unnecessaryvirtual.log", ios::app | ios::out); + myfile << output; + myfile.close(); + } + bool shouldVisitTemplateInstantiations () const { return true; } - bool VisitCXXRecordDecl( const CXXRecordDecl* decl ); bool VisitCXXMethodDecl( const CXXMethodDecl* decl ); - bool VisitCXXConstructExpr( const CXXConstructExpr* expr ); - void printTemplateInstantiations( const CXXRecordDecl *decl ); + bool VisitCallExpr(CallExpr* ); +private: + std::string fullyQualifiedName(const FunctionDecl* functionDecl); }; static std::string niceName(const CXXMethodDecl* functionDecl) @@ -64,147 +85,48 @@ static std::string niceName(const CXXMethodDecl* functionDecl) return s; } -static bool startsWith(const std::string& s, const char* other) -{ - return s.compare(0, strlen(other), other) == 0; -} - -static bool isStandardStuff(const std::string& s) -{ - // ignore UNO interface definitions, cannot change those - return startsWith(s, "com::sun::star::") - // ignore stuff in the C++ stdlib and boost - || startsWith(s, "std::") || startsWith(s, "boost::") || startsWith(s, "__gnu_debug::") - // can't change our rtl layer - || startsWith(s, "rtl::"); -} - -void UnnecessaryVirtual::printTemplateInstantiations( const CXXRecordDecl *recordDecl ) +std::string UnnecessaryVirtual::fullyQualifiedName(const FunctionDecl* functionDecl) { - for(auto functionDecl = recordDecl->method_begin(); - functionDecl != recordDecl->method_end(); ++functionDecl) - { - if (!functionDecl->isUserProvided() || !functionDecl->isVirtual()) { - continue; - } - if (isa(*functionDecl)) { - continue; - } - std::string aNiceName = niceName(*functionDecl); - if (isStandardStuff(aNiceName)) { - continue; - } - if (functionDecl->size_overridden_methods() == 0) { - cout << "definition:\t" << aNiceName << endl; - } else { - for (auto iter = functionDecl->begin_overridden_methods(); - iter != functionDecl->end_overridden_methods(); ++iter) - { - const CXXMethodDecl *pOverriddenMethod = *iter; - // we only care about the first level override to establish that a virtual qualifier was useful. - if (pOverriddenMethod->isPure() || pOverriddenMethod->size_overridden_methods() == 0) { - std::string aOverriddenNiceName = niceName(pOverriddenMethod); - if (isStandardStuff(aOverriddenNiceName)) { - continue; - } - cout << "overriding:\t" << aOverriddenNiceName << endl; - } - } - } - } - for(auto baseSpecifier = recordDecl->bases_begin(); - baseSpecifier != recordDecl->bases_end(); ++baseSpecifier) - { - QualType qt = baseSpecifier->getType().getDesugaredType(compiler.getASTContext()); - if (!qt->isRecordType()) { - continue; - } - const CXXRecordDecl *pSuperclassCXXRecordDecl = qt->getAsCXXRecordDecl(); - std::string aNiceName = pSuperclassCXXRecordDecl->getQualifiedNameAsString(); - if (isStandardStuff(aNiceName)) { - continue; - } - printTemplateInstantiations(pSuperclassCXXRecordDecl); + std::string ret = compat::getReturnType(*functionDecl).getCanonicalType().getAsString(); + ret += " "; + if (isa(functionDecl)) { + const CXXRecordDecl* recordDecl = dyn_cast(functionDecl)->getParent(); + ret += recordDecl->getQualifiedNameAsString(); + ret += "::"; + } + ret += functionDecl->getNameAsString() + "("; + bool bFirst = true; + for (const ParmVarDecl *pParmVarDecl : functionDecl->params()) { + if (bFirst) + bFirst = false; + else + ret += ","; + ret += pParmVarDecl->getType().getCanonicalType().getAsString(); } -} - -// I need to check construct expressions to see if we are instantiating any templates -// which will effectively generate new methods -bool UnnecessaryVirtual::VisitCXXConstructExpr( const CXXConstructExpr* constructExpr ) -{ - if (ignoreLocation(constructExpr)) { - return true; + ret += ")"; + if (isa(functionDecl) && dyn_cast(functionDecl)->isConst()) { + ret += " const"; } - const CXXConstructorDecl* pConstructorDecl = constructExpr->getConstructor(); - const CXXRecordDecl* recordDecl = pConstructorDecl->getParent(); - printTemplateInstantiations(recordDecl); - return true; -} -// I need to visit class definitions, so I can scan through the classes they extend to check if -// we have any template instantiations that will create new methods -bool UnnecessaryVirtual::VisitCXXRecordDecl( const CXXRecordDecl* recordDecl ) -{ - if (ignoreLocation(recordDecl)) { - return true; - } - if(!recordDecl->hasDefinition()) { - return true; - } - // ignore uninstantiated templates - if (recordDecl->getTemplateInstantiationPattern()) { - return true; - } - // ignore stuff that forms part of the stable URE interface - if (isInUnoIncludeFile(compiler.getSourceManager().getSpellingLoc( - recordDecl->getLocation()))) { - return true; - } - for(auto baseSpecifier = recordDecl->bases_begin(); - baseSpecifier != recordDecl->bases_end(); ++baseSpecifier) - { - QualType qt = baseSpecifier->getType().getDesugaredType(compiler.getASTContext()); - if (!qt->isRecordType()) { - continue; - } - const CXXRecordDecl *pSuperclassCXXRecordDecl = qt->getAsCXXRecordDecl(); - printTemplateInstantiations(pSuperclassCXXRecordDecl); - } - return true; + return ret; } -bool UnnecessaryVirtual::VisitCXXMethodDecl( const CXXMethodDecl* functionDecl ) +bool UnnecessaryVirtual::VisitCXXMethodDecl( const CXXMethodDecl* methodDecl ) { - if (ignoreLocation(functionDecl)) { - return true; - } - functionDecl = functionDecl->getCanonicalDecl(); - // ignore uninstantiated template methods - if (functionDecl->getTemplatedKind() != FunctionDecl::TemplatedKind::TK_NonTemplate - || functionDecl->getParent()->getDescribedClassTemplate() != nullptr) { - return true; - } - // ignore stuff that forms part of the stable URE interface - if (isInUnoIncludeFile(compiler.getSourceManager().getSpellingLoc( - functionDecl->getNameInfo().getLoc()))) { - return true; - } - if (isStandardStuff(functionDecl->getParent()->getQualifiedNameAsString())) { - return true; - } + methodDecl = methodDecl->getCanonicalDecl(); - std::string aNiceName = niceName(functionDecl); + std::string aNiceName = niceName(methodDecl); // for destructors, we need to check if any of the superclass' destructors are virtual - if (isa(functionDecl)) { + if (isa(methodDecl)) { /* TODO I need to check if the base class has any virtual functions, since overriding classes will simply get a compiler-provided virtual destructor by default. - if (!functionDecl->isVirtual() && !functionDecl->isPure()) { + if (!methodDecl->isVirtual() && !methodDecl->isPure()) { return true; } std::set overriddenSet; - const CXXRecordDecl *pRecordDecl = functionDecl->getParent(); + const CXXRecordDecl *pRecordDecl = methodDecl->getParent(); for(auto baseSpecifier = pRecordDecl->bases_begin(); baseSpecifier != pRecordDecl->bases_end(); ++baseSpecifier) { @@ -227,32 +149,77 @@ bool UnnecessaryVirtual::VisitCXXMethodDecl( const CXXMethodDecl* functionDecl ) return true; } - if (!functionDecl->isVirtual()) { + if (!methodDecl->isVirtual()) { return true; } - if (isStandardStuff(aNiceName)) { - return true; - } - if (functionDecl->size_overridden_methods() == 0) { - cout << "definition:\t" << aNiceName << endl; + if (methodDecl->size_overridden_methods() == 0) { + // ignore stuff that forms part of the stable URE interface + if (isInUnoIncludeFile(compiler.getSourceManager().getSpellingLoc( + methodDecl->getNameInfo().getLoc()))) { + return true; + } + // ignore templates and template instantiations, + // I just cannot get clang to give me decent overriding method data out of them + if (methodDecl->getParent()->getDescribedClassTemplate() + || methodDecl->getParent()->getTemplateInstantiationPattern()) + return true; + if (aNiceName.find("processOpCode2") != std::string::npos) + { + methodDecl->dump(); + cout << "definition " << aNiceName << endl; + } + definitionSet.insert(aNiceName); } else { - for (auto iter = functionDecl->begin_overridden_methods(); - iter != functionDecl->end_overridden_methods(); ++iter) + for (auto iter = methodDecl->begin_overridden_methods(); + iter != methodDecl->end_overridden_methods(); ++iter) { - const CXXMethodDecl *pOverriddenMethod = *iter; + const CXXMethodDecl *overriddenMethod = *iter; // we only care about the first level override to establish that a virtual qualifier was useful. - if (pOverriddenMethod->isPure() || pOverriddenMethod->size_overridden_methods() == 0) { - std::string aOverriddenNiceName = niceName(pOverriddenMethod); - if (isStandardStuff(aOverriddenNiceName)) { - continue; - } - cout << "overriding:\t" << aOverriddenNiceName << endl; + if (overriddenMethod->isPure() || overriddenMethod->size_overridden_methods() == 0) { + std::string aOverriddenNiceName = niceName(overriddenMethod); + overridingSet.insert(aOverriddenNiceName); + if (aNiceName.find("processOpCode2") != std::string::npos) + { + methodDecl->dump(); + cout << "overriding " << aNiceName << endl; + } } } } return true; } +// prevent recursive templates from blowing up the stack +static std::set traversedFunctionSet; + +bool UnnecessaryVirtual::VisitCallExpr(CallExpr* expr) +{ + // Note that I don't ignore ANYTHING here, because I want to get calls to my code that result + // from template instantiation deep inside the STL and other external code + + FunctionDecl* calleeFunctionDecl = expr->getDirectCallee(); + if (calleeFunctionDecl == nullptr) { + Expr* callee = expr->getCallee()->IgnoreParenImpCasts(); + DeclRefExpr* dr = dyn_cast(callee); + if (dr) { + calleeFunctionDecl = dyn_cast(dr->getDecl()); + if (calleeFunctionDecl) + goto gotfunc; + } + return true; + } + +gotfunc: + // if we see a call to a function, it may effectively create new code, + // if the function is templated. However, if we are inside a template function, + // calling another function on the same template, the same problem occurs. + // Rather than tracking all of that, just traverse anything we have not already traversed. + if (traversedFunctionSet.insert(fullyQualifiedName(calleeFunctionDecl)).second) + TraverseFunctionDecl(calleeFunctionDecl); + + return true; +} + loplugin::Plugin::Registration< UnnecessaryVirtual > X("unnecessaryvirtual", false); diff --git a/compilerplugins/clang/store/unnecessaryvirtual.py b/compilerplugins/clang/store/unnecessaryvirtual.py index 4bce4dea7a70..e05f16c4d84a 100755 --- a/compilerplugins/clang/store/unnecessaryvirtual.py +++ b/compilerplugins/clang/store/unnecessaryvirtual.py @@ -1,60 +1,13 @@ #!/usr/bin/python import sys +import io definitionSet = set() overridingSet = set() -# things we need to exclude for reasons like : -# - we can't see the override because it's a MS-Windows-only thing. -# - they involve function template instantiations, which I can't handle -exclusionSet = set( - "basegfx::unotools::UnoPolyPolygon::void-modifying()const", - "SalLayout::_Bool-DrawTextSpecial(class SalGraphics &,sal_uInt32,)const", - "SalLayout::_Bool-IsKashidaPosValid(int,)const", - "SalLayout::void-DisableGlyphInjection(_Bool,)", - "SalObject::void-Enable(_Bool,)", - "PropertyWrapperBase::void-SetValue(const ::com::sun::star::uno::Any &,)" - "canvas::IColorBuffer::enum canvas::IColorBuffer::Format-getFormat()const", - "canvas::IColorBuffer::sal_uInt32-getHeight()const", - "canvas::IColorBuffer::sal_uInt32-getStride()const", - "canvas::IColorBuffer::sal_uInt32-getWidth()const", - "canvas::IColorBuffer::sal_uInt8 *-lock()const", - "canvas::IColorBuffer::void-unlock()const", - "canvas::IRenderModule::::basegfx::B2IVector-getPageSize()", - "canvas::IRenderModule::::boost::shared_ptr-createSurface(const ::basegfx::B2IVector &,)", - "canvas::IRenderModule::_Bool-isError()", - "canvas::IRenderModule::void-beginPrimitive(enum canvas::IRenderModule::PrimitiveType,)", - "canvas::IRenderModule::void-endPrimitive()", - "canvas::IRenderModule::void-lock()const", - "canvas::IRenderModule::void-pushVertex(const struct canvas::Vertex &,)", - "canvas::IRenderModule::void-unlock()const", - "canvas::ISurface::_Bool-isValid()", - "canvas::ISurface::_Bool-selectTexture()", - "canvas::ISurface::_Bool-update(const ::basegfx::B2IPoint &,const ::basegfx::B2IRange &,struct canvas::IColorBuffer &,)", - "SalFrame::void-Flush(const class Rectangle &,)", - "SalFrame::void-SetRepresentedURL(const class rtl::OUString &,)", - "SalLayout::_Bool-DrawTextSpecial(class SalGraphics &,sal_uInt32,)const", - "SalLayout::_Bool-GetBoundRect(class SalGraphics &,class Rectangle &,)const", - "SalLayout::_Bool-IsKashidaPosValid(int,)const", - "SalLayout::void-DisableGlyphInjection(_Bool,)", - "writerfilter::ooxml::OOXMLFactory_ns::Id-getResourceId(Id,sal_Int32,)", - "writerfilter::ooxml::OOXMLFactory_ns::_Bool-getElementId(Id,Id,enum writerfilter::ooxml::ResourceType_t &,Id &,)", - "writerfilter::ooxml::OOXMLFactory_ns::_Bool-getListValue(Id,const class rtl::OUString &,sal_uInt32 &,)", - "writerfilter::ooxml::OOXMLFactory_ns::const struct writerfilter::ooxml::AttributeInfo *-getAttributeInfoArray(Id,)", - "sd::ZeroconfService::void-clear()", - "sd::ZeroconfService::void-setup()", - "slideshow::internal::EnumAnimation::ValueType-getUnderlyingValue()const", - "slideshow::internal::EnumAnimation::_Bool-operator()(ValueType,)", - "basegfx::unotools::UnoPolyPolygon::void-modifying()const", - "DdeTopic::_Bool-Execute(const class rtl::OUString *,)", - "DdeTopic::class DdeData *-Get(enum SotClipboardFormatId,)", - "DdeTopic::_Bool-Put(const class DdeData *,)", - "DdeTopic::_Bool-MakeItem(const class rtl::OUString &,)", - "DdeTopic::_Bool-StartAdviseLoop()", - ) -with open(sys.argv[1]) as txt: +with io.open(sys.argv[1], "rb", buffering=1024*1024) as txt: for line in txt: if line.startswith("definition:\t"): @@ -67,10 +20,8 @@ with open(sys.argv[1]) as txt: clazzName = line[idx1+1 : len(line)-1] overridingSet.add(clazzName) -for clazz in sorted(definitionSet - overridingSet - exclusionSet): - # these involve function template instantiations, which I can't handle - if not clazz.startswith("basebmp::BitmapDevice::"): - print clazz +for clazz in sorted(definitionSet - overridingSet): + print clazz # add an empty line at the end to make it easier for the removevirtuals plugin to mmap() the output file print -- cgit v1.2.3