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 <ci@libreoffice.org>
Reviewed-by: Noel Grandin <noelgrandin@gmail.com>

2 files changed, 118 insertions, 200 deletions

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 <set>
 #include "plugin.hxx"
 #include "compat.hxx"
+#include <fstream>
 
 /**
 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<std::string> definitionSet;
+static std::set<std::string> overridingSet;
+
 class UnnecessaryVirtual:
     public RecursiveASTVisitor<UnnecessaryVirtual>, 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<CXXDestructorDecl>(*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<CXXMethodDecl>(functionDecl)) {
+        const CXXRecordDecl* recordDecl = dyn_cast<CXXMethodDecl>(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<CXXMethodDecl>(functionDecl) && dyn_cast<CXXMethodDecl>(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<CXXDestructorDecl>(functionDecl)) {
+    if (isa<CXXDestructorDecl>(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<std::string> 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<std::string> 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<DeclRefExpr>(callee);
+        if (dr) {
+            calleeFunctionDecl = dyn_cast<FunctionDecl>(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<ISurface>-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