From 602647c2417e0e19e44f9c35a49fbb88ff8ac261 Mon Sep 17 00:00:00 2001 From: Noel Grandin Date: Mon, 8 Aug 2016 09:57:25 +0200 Subject: loplugin:unnecessaryvirtual Change-Id: If25d9307efda5f57b0f80a0cf5c2c5cab6a752d6 Reviewed-on: https://gerrit.libreoffice.org/27981 Tested-by: Jenkins Reviewed-by: Noel Grandin --- compilerplugins/clang/store/unnecessaryvirtual.cxx | 228 --------------------- compilerplugins/clang/store/unnecessaryvirtual.py | 28 --- compilerplugins/clang/unnecessaryvirtual.cxx | 228 +++++++++++++++++++++ compilerplugins/clang/unnecessaryvirtual.py | 37 ++++ 4 files changed, 265 insertions(+), 256 deletions(-) delete mode 100644 compilerplugins/clang/store/unnecessaryvirtual.cxx delete mode 100755 compilerplugins/clang/store/unnecessaryvirtual.py create mode 100644 compilerplugins/clang/unnecessaryvirtual.cxx create mode 100755 compilerplugins/clang/unnecessaryvirtual.py (limited to 'compilerplugins') diff --git a/compilerplugins/clang/store/unnecessaryvirtual.cxx b/compilerplugins/clang/store/unnecessaryvirtual.cxx deleted file mode 100644 index 463fc233e84e..000000000000 --- a/compilerplugins/clang/store/unnecessaryvirtual.cxx +++ /dev/null @@ -1,228 +0,0 @@ -/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*- */ -/* - * This file is part of the LibreOffice project. - * - * This Source Code Form is subject to the terms of the Mozilla Public - * License, v. 2.0. If a copy of the MPL was not distributed with this - * file, You can obtain one at http://mozilla.org/MPL/2.0/. - */ - -#include -#include -#include -#include -#include "plugin.hxx" -#include "compat.hxx" -#include - -/** -Dump a list of virtual methods and a list of methods overriding virtual methods. -Then we will post-process the 2 lists and find the set of virtual methods which don't need to be virtual. - -The process goes something like this: - $ make check - $ 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 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()); - - // 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 VisitCXXMethodDecl( const CXXMethodDecl* decl ); - bool VisitCallExpr(CallExpr* ); -private: - std::string fullyQualifiedName(const FunctionDecl* functionDecl); -}; - -std::string niceName(const CXXMethodDecl* functionDecl) -{ - std::string s = - functionDecl->getParent()->getQualifiedNameAsString() + "::" - + compat::getReturnType(*functionDecl).getAsString() + "-" - + functionDecl->getNameAsString() + "("; - for (const ParmVarDecl *pParmVarDecl : functionDecl->params()) { - s += pParmVarDecl->getType().getAsString(); - s += ","; - } - s += ")"; - if (functionDecl->isConst()) { - s += "const"; - } - return s; -} - -std::string UnnecessaryVirtual::fullyQualifiedName(const FunctionDecl* functionDecl) -{ - 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(); - } - ret += ")"; - if (isa(functionDecl) && dyn_cast(functionDecl)->isConst()) { - ret += " const"; - } - - return ret; -} - -bool UnnecessaryVirtual::VisitCXXMethodDecl( const CXXMethodDecl* methodDecl ) -{ - methodDecl = methodDecl->getCanonicalDecl(); - - 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; - } - std::set overriddenSet; - const CXXRecordDecl *pRecordDecl = methodDecl->getParent(); - for(auto baseSpecifier = pRecordDecl->bases_begin(); - baseSpecifier != pRecordDecl->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); - } - } - } - 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(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 = 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) { - 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); - -} - -/* vim:set shiftwidth=4 softtabstop=4 expandtab: */ diff --git a/compilerplugins/clang/store/unnecessaryvirtual.py b/compilerplugins/clang/store/unnecessaryvirtual.py deleted file mode 100755 index e05f16c4d84a..000000000000 --- a/compilerplugins/clang/store/unnecessaryvirtual.py +++ /dev/null @@ -1,28 +0,0 @@ -#!/usr/bin/python - -import sys -import io - -definitionSet = set() -overridingSet = set() - - -with io.open(sys.argv[1], "rb", buffering=1024*1024) as txt: - for line in txt: - - if line.startswith("definition:\t"): - idx1 = line.find("\t") - clazzName = line[idx1+1 : len(line)-1] - definitionSet.add(clazzName) - - elif line.startswith("overriding:\t"): - idx1 = line.find("\t") - clazzName = line[idx1+1 : len(line)-1] - overridingSet.add(clazzName) - -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 - diff --git a/compilerplugins/clang/unnecessaryvirtual.cxx b/compilerplugins/clang/unnecessaryvirtual.cxx new file mode 100644 index 000000000000..2964748a771c --- /dev/null +++ b/compilerplugins/clang/unnecessaryvirtual.cxx @@ -0,0 +1,228 @@ +/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*- */ +/* + * This file is part of the LibreOffice project. + * + * This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at http://mozilla.org/MPL/2.0/. + */ + +#include +#include +#include +#include +#include "plugin.hxx" +#include "compat.hxx" +#include + +/** +Dump a list of virtual methods and a list of methods overriding virtual methods. +Then we will post-process the 2 lists and find the set of virtual methods which don't need to be virtual. + +The process goes something like this: + $ make check + $ make FORCE_COMPILE_ALL=1 COMPILER_PLUGIN_TOOL='unnecessaryvirtual' check + $ ./compilerplugins/clang/unnecessaryvirtual.py + $ 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 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()); + + // 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 "/loplugin.unnecessaryvirtual.log", ios::app | ios::out); + myfile << output; + myfile.close(); + } + bool shouldVisitTemplateInstantiations () const { return true; } + + bool VisitCXXMethodDecl( const CXXMethodDecl* decl ); + bool VisitCallExpr(CallExpr* ); +private: + std::string fullyQualifiedName(const FunctionDecl* functionDecl); +}; + +std::string niceName(const CXXMethodDecl* functionDecl) +{ + std::string s = + functionDecl->getParent()->getQualifiedNameAsString() + "::" + + compat::getReturnType(*functionDecl).getAsString() + "-" + + functionDecl->getNameAsString() + "("; + for (const ParmVarDecl *pParmVarDecl : functionDecl->params()) { + s += pParmVarDecl->getType().getAsString(); + s += ","; + } + s += ")"; + if (functionDecl->isConst()) { + s += "const"; + } + return s; +} + +std::string UnnecessaryVirtual::fullyQualifiedName(const FunctionDecl* functionDecl) +{ + 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(); + } + ret += ")"; + if (isa(functionDecl) && dyn_cast(functionDecl)->isConst()) { + ret += " const"; + } + + return ret; +} + +bool UnnecessaryVirtual::VisitCXXMethodDecl( const CXXMethodDecl* methodDecl ) +{ + methodDecl = methodDecl->getCanonicalDecl(); + + 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; + } + std::set overriddenSet; + const CXXRecordDecl *pRecordDecl = methodDecl->getParent(); + for(auto baseSpecifier = pRecordDecl->bases_begin(); + baseSpecifier != pRecordDecl->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); + } + } + } + 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(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 = 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) { + 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); + +} + +/* vim:set shiftwidth=4 softtabstop=4 expandtab: */ diff --git a/compilerplugins/clang/unnecessaryvirtual.py b/compilerplugins/clang/unnecessaryvirtual.py new file mode 100755 index 000000000000..cd5613771244 --- /dev/null +++ b/compilerplugins/clang/unnecessaryvirtual.py @@ -0,0 +1,37 @@ +#!/usr/bin/python + +import sys +import io + +definitionSet = set() +overridingSet = set() + + +with io.open("loplugin.unnecessaryvirtual.log", "rb", buffering=1024*1024) as txt: + for line in txt: + + if line.startswith("definition:\t"): + idx1 = line.find("\t") + clazzName = line[idx1+1 : len(line)-1] + definitionSet.add(clazzName) + + elif line.startswith("overriding:\t"): + idx1 = line.find("\t") + clazzName = line[idx1+1 : len(line)-1] + overridingSet.add(clazzName) + +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 + # 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 + f.write("\n") + -- cgit