loplugin:unnecessaryvirtuals

Improve the plugin a little.
Create a python script to process the output.
Run it again.

Change-Id: I05c21d8a21c8f4243af739c412fda0a521f9b5f0

2 files changed, 186 insertions, 31 deletions

diff --git a/compilerplugins/clang/store/removevirtuals.cxx b/compilerplugins/clang/store/removevirtuals.cxx
index c2bf4841d30d..18aebb41e479 100644
--- a/compilerplugins/clang/store/removevirtuals.cxx
+++ b/compilerplugins/clang/store/removevirtuals.cxx
@@ -53,7 +53,7 @@ static size_t getFilesize(const char* filename)
 
 RemoveVirtuals::RemoveVirtuals(InstantiationData const & data): RewritePlugin(data)
 {
-    static const char sInputFile[] = "/home/noel/libo4/result.txt";
+    static const char sInputFile[] = "/home/noel/libo5/result.txt";
     mmapFilesize = getFilesize(sInputFile);
     //Open file
     mmapFD = open(sInputFile, O_RDONLY, 0);
@@ -120,14 +120,26 @@ bool RemoveVirtuals::VisitCXXMethodDecl( const CXXMethodDecl* functionDecl )
         return true;
     }
     if (functionDecl->isPure()) {
-        removeText(functionDecl->getSourceRange());
+        if (!removeText(functionDecl->getSourceRange())) {
+            report(
+                DiagnosticsEngine::Warning,
+                "Could not remove unused pure virtual method",
+                functionDecl->getLocStart())
+              << functionDecl->getSourceRange();
+        }
     } else {
         std::string aOrigText = rewriter->getRewrittenText(functionDecl->getSourceRange());
         size_t iVirtualTokenIndex = aOrigText.find_first_of("virtual ");
         if (iVirtualTokenIndex == std::string::npos) {
             return true;
         }
-        replaceText(functionDecl->getSourceRange(), aOrigText.replace(iVirtualTokenIndex, strlen("virtual "), ""));
+        if (!replaceText(functionDecl->getSourceRange(), aOrigText.replace(iVirtualTokenIndex, strlen("virtual "), ""))) {
+            report(
+                DiagnosticsEngine::Warning,
+                "Could not remove virtual qualifier from method",
+                functionDecl->getLocStart())
+              << functionDecl->getSourceRange();
+        }
     }
     return true;
 }
diff --git a/compilerplugins/clang/store/unnecessaryvirtual.cxx b/compilerplugins/clang/store/unnecessaryvirtual.cxx
index 0ead077473de..53688cb03a46 100644
--- a/compilerplugins/clang/store/unnecessaryvirtual.cxx
+++ b/compilerplugins/clang/store/unnecessaryvirtual.cxx
@@ -10,6 +10,7 @@
 #include <cassert>
 #include <string>
 #include <iostream>
+#include <set>
 #include "plugin.hxx"
 #include "compat.hxx"
 
@@ -20,15 +21,14 @@ 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
-  $ grep 'definition' log.txt | cut -f 2 | sort -u > definition.txt
-  $ grep 'overriding' log.txt | cut -f 2 | sort -u > overriding.txt
-  $ cat definition.txt overriding.txt | sort | uniq -u > result.txt
-  $ echo "\n" >> result.txt
-  $ for dir in *; do make  FORCE_COMPILE_ALL=1 UPDATE_FILES=$dir COMPILER_PLUGIN_TOOL='removevirtuals' $dir; done
+  $ ./compilerplugins/clang/unnecessaryvirtual.py log.txt > 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 :-)
-Notably templates tend to confuse it into removing stuff that is still needed.
+
+TODO function template instantiations are not handled
+TODO some boost bind stuff appears to confuse it, notably in the xmloff module
 */
 
 namespace {
@@ -41,8 +41,10 @@ public:
 
     virtual void run() override { TraverseDecl(compiler.getASTContext().getTranslationUnitDecl()); }
 
-    bool VisitCXXMethodDecl( const CXXMethodDecl* var );
-
+    bool VisitCXXRecordDecl( const CXXRecordDecl* decl );
+    bool VisitCXXMethodDecl( const CXXMethodDecl* decl );
+    bool VisitCXXConstructExpr( const CXXConstructExpr* expr );
+    void printTemplateInstantiations( const CXXRecordDecl *decl );
 };
 
 static std::string niceName(const CXXMethodDecl* functionDecl)
@@ -62,48 +64,189 @@ 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 )
+{
+    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);
+    }
+}
+
+// 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;
+    }
+    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;
+}
+
 bool UnnecessaryVirtual::VisitCXXMethodDecl( const CXXMethodDecl* functionDecl )
 {
     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 (!functionDecl->isVirtual()) {
+    if (isStandardStuff(functionDecl->getParent()->getQualifiedNameAsString())) {
         return true;
     }
-    // ignore UNO interface definitions, cannot change those
-    static const char cssPrefix[] = "com::sun::star";
-    if (functionDecl->getParent()->getQualifiedNameAsString().compare(0, strlen(cssPrefix), cssPrefix) == 0) {
+
+    std::string aNiceName = niceName(functionDecl);
+
+    // for destructors, we need to check if any of the superclass' destructors are virtual
+    if (isa<CXXDestructorDecl>(functionDecl)) {
+    /* 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()) {
+           return true;
+        }
+        std::set<std::string> overriddenSet;
+        const CXXRecordDecl *pRecordDecl = functionDecl->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;
     }
-    std::string aNiceName = niceName(functionDecl);
-    // Ignore virtual destructors for now.
-    // I cannot currently detect the case where we are overriding a pure virtual destructor.
-    if (dyn_cast<CXXDestructorDecl>(functionDecl)) {
+
+    if (!functionDecl->isVirtual()) {
+        return true;
+    }
+    if (isStandardStuff(aNiceName)) {
         return true;
     }
     if (functionDecl->size_overridden_methods() == 0) {
-        // ignore definition of virtual functions in templates
-//        if (functionDecl->getTemplatedKind() != FunctionDecl::TK_NonTemplate
-//           && functionDecl->getParent()->getDescribedClassTemplate() == nullptr)
-//        {
-           cout << "definition\t" << aNiceName << endl;
-//        }
+           cout << "definition:\t" << aNiceName << endl;
     } else {
-       for (CXXMethodDecl::method_iterator iter = functionDecl->begin_overridden_methods(); iter != functionDecl->end_overridden_methods(); ++iter) {
+       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->size_overridden_methods() == 0) {
-               // ignore UNO interface definitions, cannot change those
-               if (pOverriddenMethod->getParent()->getQualifiedNameAsString().compare(0, strlen(cssPrefix), cssPrefix) != 0) {
-                   std::string aOverriddenNiceName = niceName(pOverriddenMethod);
-                   cout << "overriding\t" << aOverriddenNiceName << endl;
+           if (pOverriddenMethod->isPure() || pOverriddenMethod->size_overridden_methods() == 0) {
+               std::string aOverriddenNiceName = niceName(pOverriddenMethod);
+               if (isStandardStuff(aOverriddenNiceName)) {
+                   continue;
                }
+               cout << "overriding:\t" << aOverriddenNiceName << endl;
            }
       }
     }