some global loplugin improvements

for some reason we're hitting more template AST nodes now? Anyhow,
updated singlevalfields and unusedenumconstants to cope.

For unusedfields, ignore field access inside Clone() methods, since it's
like a constructor.
Similarly for unusedmethods.

Change-Id: Icb2f76fb2f06ae5df21f9d75312e42a2800befb9
Reviewed-on: https://gerrit.libreoffice.org/45470Tested-by: Jenkins <ci@libreoffice.org>
Reviewed-by: Noel Grandin <noel.grandin@collabora.co.uk>

compilerplugins/clang/singlevalfields.cxx

@@ -256,7 +256,7 @@ bool SingleValFields::VisitMemberExpr( const MemberExpr* memberExpr )
     const Stmt* parent = getParentStmt(memberExpr);
     bool bPotentiallyAssignedTo = false;
     bool bDump = false;
-    std::string assignValue;
+    std::string assignValue = "?";
 
     // check for field being returned by non-const ref eg. Foo& getFoo() { return f; }
     if (parentFunction && parent && isa<ReturnStmt>(parent)) {
@@ -264,7 +264,6 @@ bool SingleValFields::VisitMemberExpr( const MemberExpr* memberExpr )
         if (parent2 && isa<CompoundStmt>(parent2)) {
             QualType qt = compat::getReturnType(*parentFunction).getDesugaredType(compiler.getASTContext());
             if (!qt.isConstQualified() && qt->isReferenceType()) {
-                assignValue = "?";
                 bPotentiallyAssignedTo = true;
             }
         }
@@ -279,7 +278,6 @@ bool SingleValFields::VisitMemberExpr( const MemberExpr* memberExpr )
             if (varDecl) {
                 QualType qt = varDecl->getType().getDesugaredType(compiler.getASTContext());
                 if (!qt.isConstQualified() && qt->isReferenceType()) {
-                    assignValue = "?";
                     bPotentiallyAssignedTo = true;
                     break;
                 }
@@ -310,7 +308,6 @@ bool SingleValFields::VisitMemberExpr( const MemberExpr* memberExpr )
         else if (isa<CXXOperatorCallExpr>(parent))
         {
             // FIXME need to handle this properly
-            assignValue = "?";
             bPotentiallyAssignedTo = true;
             break;
         }
@@ -330,7 +327,6 @@ bool SingleValFields::VisitMemberExpr( const MemberExpr* memberExpr )
                     const ParmVarDecl* parmVarDecl = consDecl->getParamDecl(i);
                     QualType qt = parmVarDecl->getType().getDesugaredType(compiler.getASTContext());
                     if (!qt.isConstQualified() && qt->isReferenceType()) {
-                        assignValue = "?";
                         bPotentiallyAssignedTo = true;
                     }
                     break;
@@ -348,7 +344,6 @@ bool SingleValFields::VisitMemberExpr( const MemberExpr* memberExpr )
                 assignValue = getExprValue(binaryOp->getRHS());
                 bPotentiallyAssignedTo = true;
             } else {
-                assignValue = "?";
                 bPotentiallyAssignedTo = true;
             }
             break;
@@ -375,6 +370,13 @@ bool SingleValFields::VisitMemberExpr( const MemberExpr* memberExpr )
         {
             break;
         }
+#if CLANG_VERSION >= 40000
+        else if ( isa<ArrayInitLoopExpr>(parent) )
+        {
+            bPotentiallyAssignedTo = true;
+            break;
+        }
+#endif
         else {
             bPotentiallyAssignedTo = true;
             bDump = true;

compilerplugins/clang/unusedenumconstants.cxx

@@ -184,6 +184,22 @@ try_again:
                 || isa<ExprWithCleanups>(parent))
     {
         goto try_again;
+    } else if (isa<CXXDefaultArgExpr>(parent))
+    {
+        // TODO this could be improved
+        bWrite = true;
+    } else if (isa<DeclRefExpr>(parent))
+    {
+        // slightly weird case I saw in basegfx where the enum is being used as a template param
+        bWrite = true;
+    } else if (isa<MemberExpr>(parent))
+    {
+        // slightly weird case I saw in sc where the enum is being used as a template param
+        bWrite = true;
+    } else if (isa<UnresolvedLookupExpr>(parent))
+    {
+        bRead = true;
+        bWrite = true;
     } else {
         bDump = true;
     }
@@ -191,6 +207,7 @@ try_again:
     // to let me know if I missed something
     if (bDump) {
         parent->dump();
+        declRefExpr->dump();
         report( DiagnosticsEngine::Warning,
                 "unhandled clang AST node type",
                 parent->getLocStart());

compilerplugins/clang/unusedfields.cxx

@@ -159,7 +159,7 @@ private:
                                         CalleeWrapper calleeFunctionDecl);
     llvm::Optional<CalleeWrapper> getCallee(CallExpr const *);
 
-    RecordDecl *   insideMoveOrCopyDeclParent = nullptr;
+    RecordDecl *   insideMoveOrCopyOrCloneDeclParent = nullptr;
     RecordDecl *   insideStreamOutputOperator = nullptr;
     // For reasons I do not understand, parentFunctionDecl() is not reliable, so
     // we store the parent function on the way down the AST.
@@ -361,29 +361,31 @@ bool startswith(const std::string& rStr, const char* pSubStr)
 
 bool UnusedFields::TraverseCXXConstructorDecl(CXXConstructorDecl* cxxConstructorDecl)
 {
-    auto copy = insideMoveOrCopyDeclParent;
+    auto copy = insideMoveOrCopyOrCloneDeclParent;
     if (!ignoreLocation(cxxConstructorDecl) && cxxConstructorDecl->isThisDeclarationADefinition())
     {
         if (cxxConstructorDecl->isCopyOrMoveConstructor())
-            insideMoveOrCopyDeclParent = cxxConstructorDecl->getParent();
+            insideMoveOrCopyOrCloneDeclParent = cxxConstructorDecl->getParent();
     }
     bool ret = RecursiveASTVisitor::TraverseCXXConstructorDecl(cxxConstructorDecl);
-    insideMoveOrCopyDeclParent = copy;
+    insideMoveOrCopyOrCloneDeclParent = copy;
     return ret;
 }
 
 bool UnusedFields::TraverseCXXMethodDecl(CXXMethodDecl* cxxMethodDecl)
 {
-    auto copy1 = insideMoveOrCopyDeclParent;
+    auto copy1 = insideMoveOrCopyOrCloneDeclParent;
     auto copy2 = insideFunctionDecl;
     if (!ignoreLocation(cxxMethodDecl) && cxxMethodDecl->isThisDeclarationADefinition())
     {
-        if (cxxMethodDecl->isCopyAssignmentOperator() || cxxMethodDecl->isMoveAssignmentOperator())
-            insideMoveOrCopyDeclParent = cxxMethodDecl->getParent();
+        if (cxxMethodDecl->isCopyAssignmentOperator()
+            || cxxMethodDecl->isMoveAssignmentOperator()
+            || (cxxMethodDecl->getIdentifier() && cxxMethodDecl->getName() == "Clone"))
+            insideMoveOrCopyOrCloneDeclParent = cxxMethodDecl->getParent();
     }
     insideFunctionDecl = cxxMethodDecl;
     bool ret = RecursiveASTVisitor::TraverseCXXMethodDecl(cxxMethodDecl);
-    insideMoveOrCopyDeclParent = copy1;
+    insideMoveOrCopyOrCloneDeclParent = copy1;
     insideFunctionDecl = copy2;
     return ret;
 }
@@ -435,11 +437,11 @@ bool UnusedFields::VisitMemberExpr( const MemberExpr* memberExpr )
 
 void UnusedFields::checkWriteOnly(const FieldDecl* fieldDecl, const Expr* memberExpr)
 {
-    if (insideMoveOrCopyDeclParent || insideStreamOutputOperator)
+    if (insideMoveOrCopyOrCloneDeclParent || insideStreamOutputOperator)
     {
         RecordDecl const * cxxRecordDecl1 = fieldDecl->getParent();
         // we don't care about reads from a field when inside the copy/move constructor/operator= for that field
-        if (cxxRecordDecl1 && (cxxRecordDecl1 == insideMoveOrCopyDeclParent))
+        if (cxxRecordDecl1 && (cxxRecordDecl1 == insideMoveOrCopyOrCloneDeclParent))
             return;
         // we don't care about reads when the field is being used in an output operator, this is normally
         // debug stuff
@@ -629,11 +631,11 @@ void UnusedFields::checkWriteOnly(const FieldDecl* fieldDecl, const Expr* member
 
 void UnusedFields::checkReadOnly(const FieldDecl* fieldDecl, const Expr* memberExpr)
 {
-    if (insideMoveOrCopyDeclParent)
+    if (insideMoveOrCopyOrCloneDeclParent)
     {
         RecordDecl const * cxxRecordDecl1 = fieldDecl->getParent();
         // we don't care about writes to a field when inside the copy/move constructor/operator= for that field
-        if (cxxRecordDecl1 && (cxxRecordDecl1 == insideMoveOrCopyDeclParent))
+        if (cxxRecordDecl1 && (cxxRecordDecl1 == insideMoveOrCopyOrCloneDeclParent))
             return;
     }
 
@@ -878,7 +880,7 @@ bool UnusedFields::VisitCXXConstructorDecl( const CXXConstructorDecl* cxxConstru
        return true;
 
     // we don't care about writes to a field when inside the copy/move constructor/operator= for that field
-    if (insideMoveOrCopyDeclParent && cxxConstructorDecl->getParent() == insideMoveOrCopyDeclParent)
+    if (insideMoveOrCopyOrCloneDeclParent && cxxConstructorDecl->getParent() == insideMoveOrCopyOrCloneDeclParent)
         return true;
 
     for(auto it = cxxConstructorDecl->init_begin(); it != cxxConstructorDecl->init_end(); ++it)
@@ -975,33 +977,7 @@ llvm::Optional<CalleeWrapper> UnusedFields::getCallee(CallExpr const * callExpr)
         }
     }
 
-    llvm::Optional<CalleeWrapper> ret;
-    auto callee = callExpr->getCallee()->IgnoreParenImpCasts();
-    if (isa<CXXDependentScopeMemberExpr>(callee)) // template stuff
-        return ret;
-    if (isa<UnresolvedLookupExpr>(callee)) // template stuff
-        return ret;
-    if (isa<UnresolvedMemberExpr>(callee)) // template stuff
-        return ret;
-    calleeType = calleeType->getUnqualifiedDesugaredType();
-    if (isa<TemplateSpecializationType>(calleeType)) // template stuff
-        return ret;
-    if (auto builtinType = dyn_cast<BuiltinType>(calleeType)) {
-        if (builtinType->getKind() == BuiltinType::Kind::Dependent) // template stuff
-            return ret;
-        if (builtinType->getKind() == BuiltinType::Kind::BoundMember) // template stuff
-            return ret;
-    }
-    if (isa<TemplateTypeParmType>(calleeType)) // template stuff
-        return ret;
-
-    callExpr->dump();
-    callExpr->getCallee()->getType()->dump();
-    report(
-         DiagnosticsEngine::Warning, "can't get callee",
-        callExpr->getExprLoc())
-      << callExpr->getSourceRange();
-    return ret;
+    return llvm::Optional<CalleeWrapper>();
 }
 
 loplugin::Plugin::Registration< UnusedFields > X("unusedfields", false);

compilerplugins/clang/unusedmethods.cxx

@@ -235,8 +235,16 @@ bool UnusedMethods::VisitCallExpr(CallExpr* expr)
 
 gotfunc:
 
-    // for "unused method" analysis, ignore recursive calls
-    if (currentFunctionDecl != calleeFunctionDecl)
+
+    if (currentFunctionDecl == calleeFunctionDecl)
+        ; // for "unused method" analysis, ignore recursive calls
+    else if (currentFunctionDecl
+            && currentFunctionDecl->getIdentifier()
+            && currentFunctionDecl->getName() == "Clone"
+            && currentFunctionDecl->getParent() == calleeFunctionDecl->getParent()
+            && isa<CXXConstructorDecl>(calleeFunctionDecl))
+        ; // if we are inside Clone(), ignore calls to the same class's constructor
+    else
         logCallToRootMethods(calleeFunctionDecl, callSet);
 
     const Stmt* parent = getParentStmt(expr);