new loplugin to find public methods that can be private

based on the unusedmethods plugin, which I should probably rename at
some point

Change-Id: If197423c59d4350ea1fdc69e99d24b631d9751b9

2 files changed, 131 insertions, 28 deletions

diff --git a/compilerplugins/clang/unusedmethods.cxx b/compilerplugins/clang/unusedmethods.cxx
index 6f724f8b0e36..b05ffe3dae5d 100644
--- a/compilerplugins/clang/unusedmethods.cxx
+++ b/compilerplugins/clang/unusedmethods.cxx
@@ -16,8 +16,14 @@
 #include "compat.hxx"
 
 /**
-Dump a list of calls to methods, and a list of method definitions.
-Then we will post-process the 2 lists and find the set of unused methods.
+This plugin performs 3 different analyses:
+
+(1) Find unused methods
+(2) Find methods whose return types are never evaluated
+(3) Find methods which are public, but are never called from outside the class i.e. they can be private
+
+It does so, by dumping various call/definition/use info to a log file.
+Then we will post-process the various lists and find the set of unused methods.
 
 Be warned that it produces around 5G of log file.
 
@@ -41,6 +47,7 @@ namespace {
 
 struct MyFuncInfo
 {
+    std::string access;
     std::string returnType;
     std::string nameAndParams;
     std::string sourceLocation;
@@ -58,16 +65,17 @@ struct MyFuncInfo
 
 
 // try to limit the voluminous output a little
+
+// for the "unused method" analysis
 static std::set<MyFuncInfo> callSet;
-static std::set<MyFuncInfo> usedReturnSet;
 static std::set<MyFuncInfo> definitionSet;
+// for the "unused return type" analysis
+static std::set<MyFuncInfo> usedReturnSet;
+// for the "unnecessary public" analysis
+static std::set<MyFuncInfo> publicDefinitionSet;
+static std::set<MyFuncInfo> calledFromOutsideSet;
 
 
-static bool startswith(const std::string& s, const std::string& prefix)
-{
-    return s.rfind(prefix,0) == 0;
-}
-
 class UnusedMethods:
     public RecursiveASTVisitor<UnusedMethods>, public loplugin::Plugin
 {
@@ -80,17 +88,19 @@ 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 MyFuncInfo & s : definitionSet)
+            output += "definition:\t" + s.access + "\t" + s.returnType + "\t" + s.nameAndParams + "\t" + s.sourceLocation + "\n";
+        // for the "unused method" analysis
         for (const MyFuncInfo & s : callSet)
             output += "call:\t" + s.returnType + "\t" + s.nameAndParams + "\n";
+        // for the "unused return type" analysis
         for (const MyFuncInfo & s : usedReturnSet)
             output += "usedReturn:\t" + s.returnType + "\t" + s.nameAndParams + "\n";
-        for (const MyFuncInfo & s : definitionSet)
-        {
-            //treat all UNO interfaces as having been called, since they are part of our external ABI
-            if (!startswith(s.nameAndParams, "com::sun::star::"))
-                output += "definition:\t" + s.returnType + "\t" + s.nameAndParams + "\t" + s.sourceLocation + "\n";
-        }
+        // for the "unnecessary public" analysis
+        for (const MyFuncInfo & s : calledFromOutsideSet)
+            output += "outside:\t" + s.returnType + "\t" + s.nameAndParams + "\n";
         ofstream myfile;
         myfile.open( SRCDIR "/unusedmethods.log", ios::app | ios::out);
         myfile << output;
@@ -121,6 +131,13 @@ MyFuncInfo UnusedMethods::niceName(const FunctionDecl* functionDecl)
 #endif
 
     MyFuncInfo aInfo;
+    switch (functionDecl->getAccess())
+    {
+    case AS_public: aInfo.access = "public"; break;
+    case AS_private: aInfo.access = "private"; break;
+    case AS_protected: aInfo.access = "protected"; break;
+    default: aInfo.access = "unknown"; break;
+    }
     aInfo.returnType = compat::getReturnType(*functionDecl).getCanonicalType().getAsString();
 
     if (isa<CXXMethodDecl>(functionDecl)) {
@@ -201,6 +218,33 @@ void UnusedMethods::logCallToRootMethods(const FunctionDecl* functionDecl, std::
 // prevent recursive templates from blowing up the stack
 static std::set<std::string> traversedFunctionSet;
 
+const Decl* get_DeclContext_from_Stmt(ASTContext& context, const Stmt& stmt)
+{
+  auto it = context.getParents(stmt).begin();
+
+  if (it == context.getParents(stmt).end())
+      return nullptr;
+
+  const Decl *aDecl = it->get<Decl>();
+  if (aDecl)
+      return aDecl;
+
+  const Stmt *aStmt = it->get<Stmt>();
+  if (aStmt)
+      return get_DeclContext_from_Stmt(context, *aStmt);
+
+  return nullptr;
+}
+
+static const FunctionDecl* get_top_FunctionDecl_from_Stmt(ASTContext& context, const Stmt& stmt)
+{
+  const Decl *decl = get_DeclContext_from_Stmt(context, stmt);
+  if (decl)
+      return static_cast<const FunctionDecl*>(decl->getNonClosureContext());
+
+  return nullptr;
+}
+
 bool UnusedMethods::VisitCallExpr(CallExpr* expr)
 {
     // Note that I don't ignore ANYTHING here, because I want to get calls to my code that result
@@ -232,10 +276,22 @@ gotfunc:
 
     logCallToRootMethods(calleeFunctionDecl, callSet);
 
+    const Stmt* parent = parentStmt(expr);
+
+    // Now do the checks necessary for the "unnecessary public" analysis
+    CXXMethodDecl* calleeMethodDecl = dyn_cast<CXXMethodDecl>(calleeFunctionDecl);
+    if (calleeMethodDecl && calleeMethodDecl->getAccess() == AS_public)
+    {
+        const FunctionDecl* parentFunctionDecl = get_top_FunctionDecl_from_Stmt(compiler.getASTContext(), *expr);
+        if (parentFunctionDecl && parentFunctionDecl != calleeFunctionDecl) {
+            calledFromOutsideSet.insert(niceName(parentFunctionDecl));
+        }
+    }
+
+    // Now do the checks necessary for the "unused return value" analysis
     if (calleeFunctionDecl->getReturnType()->isVoidType()) {
         return true;
     }
-    const Stmt* parent = parentStmt(expr);
     if (!parent) {
         // we will get null parent if it's under a CXXConstructExpr node
         logCallToRootMethods(calleeFunctionDecl, usedReturnSet);
@@ -283,7 +339,13 @@ bool UnusedMethods::VisitFunctionDecl( const FunctionDecl* functionDecl )
     }
 
     if( functionDecl->getLocation().isValid() && !ignoreLocation( functionDecl ))
-        definitionSet.insert(niceName(functionDecl));
+    {
+        MyFuncInfo funcInfo = niceName(functionDecl);
+        definitionSet.insert(funcInfo);
+        if (functionDecl->getAccess() == AS_public) {
+            publicDefinitionSet.insert(funcInfo);
+        }
+    }
     return true;
 }
 
diff --git a/compilerplugins/clang/unusedmethods.py b/compilerplugins/clang/unusedmethods.py
index 2f94f16bd9ca..504a97fc4e32 100755
--- a/compilerplugins/clang/unusedmethods.py
+++ b/compilerplugins/clang/unusedmethods.py
@@ -5,10 +5,13 @@ import re
 import io
 
 definitionSet = set()
+publicDefinitionSet = set()
 definitionToSourceLocationMap = dict()
 callSet = set()
-returnSet = set()
+usedReturnSet = set()
 sourceLocationSet = set()
+calledFromOutsideSet = set()
+
 # things we need to exclude for reasons like :
 # - it's a weird template thingy that confuses the plugin
 exclusionSet = set([
@@ -120,19 +123,35 @@ with io.open(sys.argv[1], "rb", buffering=1024*1024) as txt:
         if line.startswith("definition:\t"):
             idx1 = line.find("\t",12)
             idx2 = line.find("\t",idx1+1)
-            funcInfo = (normalizeTypeParams(line[12:idx1]), normalizeTypeParams(line[idx1+1:idx2]))
+            idx3 = line.find("\t",idx2+1)
+            access = line[12:idx1]
+            returnType = line[idx1+1:idx2]
+            nameAndParams = line[idx2+1:idx3]
+            sourceLocation = line[idx3+1:].strip()
+            funcInfo = (normalizeTypeParams(returnType), normalizeTypeParams(nameAndParams))
             definitionSet.add(funcInfo)
-            definitionToSourceLocationMap[funcInfo] = line[idx2+1:].strip()
+            if access == "public":
+                publicDefinitionSet.add(funcInfo)
+            definitionToSourceLocationMap[funcInfo] = sourceLocation
         elif line.startswith("call:\t"):
             idx1 = line.find("\t",6)
-            callSet.add((normalizeTypeParams(line[6:idx1]), normalizeTypeParams(line[idx1+1:].strip())))
+            returnType = line[6:idx1]
+            nameAndParams = line[idx1+1:].strip()
+            callSet.add((normalizeTypeParams(returnType), normalizeTypeParams(nameAndParams)))
         elif line.startswith("usedReturn:\t"):
             idx1 = line.find("\t",12)
-            returnSet.add((normalizeTypeParams(line[12:idx1]), normalizeTypeParams(line[idx1+1:].strip())))
+            returnType = line[12:idx1]
+            nameAndParams = line[idx1+1:].strip()
+            usedReturnSet.add((normalizeTypeParams(returnType), normalizeTypeParams(nameAndParams)))
+        elif line.startswith("calledFromOutsideSet:\t"):
+            idx1 = line.find("\t",22)
+            returnType = line[22:idx1]
+            nameAndParams = line[idx1+1:].strip()
+            calledFromOutsideSet.add((normalizeTypeParams(returnType), normalizeTypeParams(nameAndParams)))
 
-# Invert the definitionToSourceLocationMap
+# Invert the definitionToSourceLocationMap.
 # If we see more than one method at the same sourceLocation, it's being autogenerated as part of a template
-# and we should just ignore
+# and we should just ignore it.
 sourceLocationToDefinitionMap = {}
 for k, v in definitionToSourceLocationMap.iteritems():
     sourceLocationToDefinitionMap[v] = sourceLocationToDefinitionMap.get(v, [])
@@ -253,11 +272,9 @@ tmp1list = sorted(tmp1set, key=lambda v: natural_sort_key(v[1]))
 tmp2set = set()
 for d in definitionSet:
     clazz = d[0] + " " + d[1]
-    if clazz in exclusionSet:
-        continue
-    if d in returnSet:
+    if d in usedReturnSet:
         continue
-    if isOtherConstness(d, returnSet):
+    if isOtherConstness(d, usedReturnSet):
         continue
     if d[0] == "void":
         continue
@@ -287,7 +304,31 @@ for d in definitionSet:
 # sort results by name and line number
 tmp2list = sorted(tmp2set, key=lambda v: natural_sort_key(v[1]))
     
-for t in tmp2list:
+#for t in tmp2list:
+#    print t[1]
+#    print "    ", t[0]
+
+
+# -------------------------------------------
+# Do the "unnecessary public" part
+# -------------------------------------------
+
+tmp3set = set()
+for d in publicDefinitionSet:
+    clazz = d[0] + " " + d[1]
+    if d in calledFromOutsideSet:
+        continue
+    if isOtherConstness(d, calledFromOutsideSet):
+        continue
+    # ignore external code
+    if definitionToSourceLocationMap[d].startswith("external/"):
+       continue
+    tmp3set.add((clazz, definitionToSourceLocationMap[d]))
+
+# sort results by name and line number
+tmp3list = sorted(tmp3set, key=lambda v: natural_sort_key(v[1]))
+    
+for t in tmp3list:
     print t[1]
     print "    ", t[0]