From 5f77e6e9309cab4633fa8211f9788af9a9a793c9 Mon Sep 17 00:00:00 2001 From: Noel Grandin Date: Thu, 3 Nov 2016 15:12:56 +0200 Subject: update loplugin:unnnecessaryvirtual to handler destructors and update modules writerfilter..xmloff with the resulting changes Change-Id: I54d19c22ddb0ff579b32e4934d266c925b19305c Reviewed-on: https://gerrit.libreoffice.org/30530 Tested-by: Jenkins Reviewed-by: Noel Grandin --- compilerplugins/clang/unnecessaryvirtual.cxx | 136 ++++++++++++--------------- compilerplugins/clang/unnecessaryvirtual.py | 80 ++++++++++------ compilerplugins/clang/unusedenumvalues.py | 6 -- 3 files changed, 107 insertions(+), 115 deletions(-) (limited to 'compilerplugins') diff --git a/compilerplugins/clang/unnecessaryvirtual.cxx b/compilerplugins/clang/unnecessaryvirtual.cxx index 1ecc47114a0b..82f91c71f1a7 100644 --- a/compilerplugins/clang/unnecessaryvirtual.cxx +++ b/compilerplugins/clang/unnecessaryvirtual.cxx @@ -34,8 +34,19 @@ TODO does not find destructors that don't need to be virtual namespace { +struct MyFuncInfo +{ + std::string name; + std::string sourceLocation; + +}; +bool operator < (const MyFuncInfo &lhs, const MyFuncInfo &rhs) +{ + return lhs.name < rhs.name; +} + // try to limit the voluminous output a little -static std::set definitionSet; +static std::set definitionSet; static std::set overridingSet; class UnnecessaryVirtual: @@ -51,8 +62,8 @@ public: // 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 MyFuncInfo & s : definitionSet) + output += "definition:\t" + s.name + "\t" + s.sourceLocation + "\n"; for (const std::string & s : overridingSet) output += "overriding:\t" + s + "\n"; ofstream myfile; @@ -61,11 +72,12 @@ public: myfile.close(); } bool shouldVisitTemplateInstantiations () const { return true; } + bool shouldVisitImplicitCode() const { return true; } bool VisitCXXMethodDecl( const CXXMethodDecl* decl ); - bool VisitCallExpr(CallExpr* ); private: std::string fullyQualifiedName(const FunctionDecl* functionDecl); + std::string toString(SourceLocation loc); }; std::string niceName(const CXXMethodDecl* functionDecl) @@ -87,6 +99,16 @@ std::string niceName(const CXXMethodDecl* functionDecl) std::string UnnecessaryVirtual::fullyQualifiedName(const FunctionDecl* functionDecl) { + if (functionDecl->getInstantiatedFromMemberFunction()) + functionDecl = functionDecl->getInstantiatedFromMemberFunction(); + else if (functionDecl->getClassScopeSpecializationPattern()) + functionDecl = functionDecl->getClassScopeSpecializationPattern(); +// workaround clang-3.5 issue +#if CLANG_VERSION >= 30600 + else if (functionDecl->getTemplateInstantiationPattern()) + functionDecl = functionDecl->getTemplateInstantiationPattern(); +#endif + std::string ret = compat::getReturnType(*functionDecl).getCanonicalType().getAsString(); ret += " "; if (isa(functionDecl)) { @@ -113,110 +135,68 @@ std::string UnnecessaryVirtual::fullyQualifiedName(const FunctionDecl* functionD bool UnnecessaryVirtual::VisitCXXMethodDecl( const CXXMethodDecl* methodDecl ) { + if (ignoreLocation(methodDecl)) { + return true; + } + if (!methodDecl->isThisDeclarationADefinition() || + !methodDecl->isVirtual() || + methodDecl->isDeleted()) + { + return true; + } methodDecl = methodDecl->getCanonicalDecl(); + // ignore stuff that forms part of the stable URE interface + if (isInUnoIncludeFile(methodDecl)) { + return true; + } std::string aNiceName = niceName(methodDecl); // for destructors, we need to check if any of the superclass' destructors are virtual 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 (!methodDecl->isVirtual() && !methodDecl->isPure()) { - return true; + const CXXRecordDecl* cxxRecordDecl = methodDecl->getParent(); + if (cxxRecordDecl->getNumBases() == 0) { + definitionSet.insert( { aNiceName, toString( methodDecl->getLocation() ) } ); + return true; } - std::set overriddenSet; - const CXXRecordDecl *pRecordDecl = methodDecl->getParent(); - for(auto baseSpecifier = pRecordDecl->bases_begin(); - baseSpecifier != pRecordDecl->bases_end(); ++baseSpecifier) + for(auto baseSpecifier = cxxRecordDecl->bases_begin(); + baseSpecifier != cxxRecordDecl->bases_end(); ++baseSpecifier) { if (baseSpecifier->getType()->isRecordType()) { - const CXXRecordDecl *pSuperclassCXXRecordDecl = baseSpecifier->getType()->getAsCXXRecordDecl(); - if (pSuperclassCXXRecordDecl->getDestructor()) - { - std::string aOverriddenNiceName = niceName(pSuperclassCXXRecordDecl->getDestructor()); - overriddenSet.insert(aOverriddenNiceName); - } + const CXXRecordDecl* superclassCXXRecordDecl = baseSpecifier->getType()->getAsCXXRecordDecl(); + std::string aOverriddenNiceName = niceName(superclassCXXRecordDecl->getDestructor()); + overridingSet.insert(aOverriddenNiceName); } } - if (overriddenSet.empty()) { - cout << "definition:\t" << aNiceName << endl; - } else { - for(std::string s : overriddenSet) - cout << "overriding:\t" << s << endl; - }*/ return true; } - if (!methodDecl->isVirtual()) { - return true; - } if (methodDecl->size_overridden_methods() == 0) { - // ignore stuff that forms part of the stable URE interface - if (isInUnoIncludeFile(methodDecl)) { - 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); + definitionSet.insert( { aNiceName, toString( methodDecl->getLocation() ) } ); } else { for (auto iter = methodDecl->begin_overridden_methods(); iter != methodDecl->end_overridden_methods(); ++iter) { const CXXMethodDecl *overriddenMethod = *iter; // we only care about the first level override to establish that a virtual qualifier was useful. - if (overriddenMethod->isPure() || overriddenMethod->size_overridden_methods() == 0) { + 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) +std::string UnnecessaryVirtual::toString(SourceLocation loc) { - // 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; + SourceLocation expansionLoc = compiler.getSourceManager().getExpansionLoc( loc ); + StringRef name = compiler.getSourceManager().getFilename(expansionLoc); + std::string sourceLocation = std::string(name.substr(strlen(SRCDIR)+1)) + ":" + std::to_string(compiler.getSourceManager().getSpellingLineNumber(expansionLoc)); + normalizeDotDotInFilePath(sourceLocation); + return sourceLocation; } diff --git a/compilerplugins/clang/unnecessaryvirtual.py b/compilerplugins/clang/unnecessaryvirtual.py index dd1ea719c7e7..651f8732758e 100755 --- a/compilerplugins/clang/unnecessaryvirtual.py +++ b/compilerplugins/clang/unnecessaryvirtual.py @@ -1,9 +1,11 @@ #!/usr/bin/python -import sys import io +import re +import sys definitionSet = set() +definitionToSourceLocationMap = dict() overridingSet = set() @@ -11,38 +13,54 @@ with io.open("loplugin.unnecessaryvirtual.log", "rb", buffering=1024*1024) as tx for line in txt: tokens = line.strip().split("\t") if tokens[0] == "definition:": - clazzName = tokens[1] - definitionSet.add(clazzName) + fullMethodName = tokens[1] + sourceLocation = tokens[2] + definitionSet.add(fullMethodName) + definitionToSourceLocationMap[fullMethodName] = sourceLocation elif tokens[0] == "overriding:": - clazzName = tokens[1] - overridingSet.add(clazzName) + fullMethodName = tokens[1] + overridingSet.add(fullMethodName) +unnecessaryVirtualSet = set() + +for clazz in (definitionSet - overridingSet): + # windows-specific stuff + if clazz.startswith("canvas::"): continue + if clazz.startswith("psp::PrinterInfoManager"): continue + if clazz.startswith("DdeTopic::"): continue + if clazz == "basegfx::unotools::UnoPolyPolygon::void-modifying()const": continue + if clazz == "SalLayout::_Bool-IsKashidaPosValid(int,)const": continue + if clazz == "SalLayout::void-DisableGlyphInjection(_Bool,)": continue + # Linux-TDF specific + if clazz == "X11SalFrame::void-updateGraphics(_Bool,)": continue + # OSX specific + if clazz == "SalFrame::void-SetRepresentedURL(const class rtl::OUString &,)": continue + if clazz == "SalMenu::_Bool-AddMenuBarButton(const struct SalMenuButtonItem &,)": continue + if clazz == "SalMenu::class Rectangle-GetMenuBarButtonRectPixel(sal_uInt16,class SalFrame *,)": continue + if clazz == "SalMenu::void-RemoveMenuBarButton(sal_uInt16,)": continue + if clazz == "SalLayout::_Bool-DrawTextSpecial(class SalGraphics &,sal_uInt32,)const": continue + # GTK < 3 + if clazz == "GtkSalDisplay::int-CaptureMouse(class SalFrame *,)": continue + # some test magic + if clazz.startswith("apitest::"): continue + # ignore external code + if definitionToSourceLocationMap[clazz].startswith("external/"): continue + + unnecessaryVirtualSet.add((clazz,definitionToSourceLocationMap[clazz] )) + + +# sort the results using a "natural order" so sequences like [item1,item2,item10] sort nicely +def natural_sort_key(s, _nsre=re.compile('([0-9]+)')): + return [int(text) if text.isdigit() else text.lower() + for text in re.split(_nsre, s)] + +# sort results by name and line number +tmp1list = sorted(unnecessaryVirtualSet, key=lambda v: natural_sort_key(v[1])) + with open("loplugin.unnecessaryvirtual.report", "wt") as f: - for clazz in sorted(definitionSet - overridingSet): - # external code - if clazz.startswith("std::"): continue - if clazz.startswith("icu_"): continue - if clazz.startswith("__cxx"): continue - # windows-specific stuff - if clazz.startswith("canvas::"): continue - if clazz.startswith("psp::PrinterInfoManager"): continue - if clazz.startswith("DdeTopic::"): continue - if clazz == "basegfx::unotools::UnoPolyPolygon::void-modifying()const": continue - if clazz == "SalLayout::_Bool-IsKashidaPosValid(int,)const": continue - if clazz == "SalLayout::void-DisableGlyphInjection(_Bool,)": continue - # Linux-TDF specific - if clazz == "X11SalFrame::void-updateGraphics(_Bool,)": continue - # OSX specific - if clazz == "SalFrame::void-SetRepresentedURL(const class rtl::OUString &,)": continue - if clazz == "SalMenu::_Bool-AddMenuBarButton(const struct SalMenuButtonItem &,)": continue - if clazz == "SalMenu::class Rectangle-GetMenuBarButtonRectPixel(sal_uInt16,class SalFrame *,)": continue - if clazz == "SalMenu::void-RemoveMenuBarButton(sal_uInt16,)": continue - if clazz == "SalLayout::_Bool-DrawTextSpecial(class SalGraphics &,sal_uInt32,)const": continue - # GTK < 3 - if clazz == "GtkSalDisplay::int-CaptureMouse(class SalFrame *,)": continue - # some test magic - if clazz.startswith("apitest::"): continue - f.write(clazz + "\n") - # add an empty line at the end to make it easier for the removevirtuals plugin to mmap() the output file + for t in tmp1list: + f.write( t[1] + "\n" ) + f.write( " " + t[0] + "\n" ) + # add an empty line at the end to make it easier for the removevirtuals plugin to mmap() the output file f.write("\n") diff --git a/compilerplugins/clang/unusedenumvalues.py b/compilerplugins/clang/unusedenumvalues.py index eb3158509562..76c9fe619eb3 100755 --- a/compilerplugins/clang/unusedenumvalues.py +++ b/compilerplugins/clang/unusedenumvalues.py @@ -108,9 +108,3 @@ with open("loplugin.unusedenumvalues.report-untouched", "wt") as f: f.write( t[1] + "\n" ) f.write( " " + t[0] + "\n" ) - - -# add an empty line at the end to make it easier for the unusedFieldsremove plugin to mmap() the output file -print - - -- cgit