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 <ci@libreoffice.org>
Reviewed-by: Noel Grandin <noel.grandin@collabora.co.uk>

3 files changed, 107 insertions, 115 deletions

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<std::string> definitionSet;
+static std::set<MyFuncInfo> definitionSet;
 static std::set<std::string> 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<CXXMethodDecl>(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<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 (!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<std::string> 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<std::string> 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<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;
+    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
-        
-