improve loplugin constparams

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

2 files changed, 35 insertions, 28 deletions

diff --git a/compilerplugins/clang/constparams.cxx b/compilerplugins/clang/constparams.cxx
index cccc454fa1af..3f1aad80b38e 100644
--- a/compilerplugins/clang/constparams.cxx
+++ b/compilerplugins/clang/constparams.cxx
@@ -76,11 +76,12 @@ public:
             {
                 continue;
             }
+            std::string fname = functionDecl->getQualifiedNameAsString();
             report(
                 DiagnosticsEngine::Warning,
-                "this parameter can be const",
+                "this parameter can be const %0",
                 compat::getBeginLoc(pParmVarDecl))
-                << pParmVarDecl->getSourceRange();
+                << fname << pParmVarDecl->getSourceRange();
             if (canonicalDecl->getLocation() != functionDecl->getLocation()) {
                 unsigned idx = pParmVarDecl->getFunctionScopeIndex();
                 const ParmVarDecl* pOther = canonicalDecl->getParamDecl(idx);
@@ -150,16 +151,8 @@ bool ConstParams::CheckTraverseFunctionDecl(FunctionDecl * functionDecl)
     if (isInUnoIncludeFile(functionDecl)) {
         return false;
     }
-    // TODO ignore template stuff for now
-    if (functionDecl->getTemplatedKind() != FunctionDecl::TK_NonTemplate) {
-        return false;
-    }
     if (functionDecl->isDeleted())
         return false;
-    if (isa<CXXMethodDecl>(functionDecl)
-        && dyn_cast<CXXMethodDecl>(functionDecl)->getParent()->getDescribedClassTemplate() != nullptr ) {
-        return false;
-    }
     // ignore virtual methods
     if (isa<CXXMethodDecl>(functionDecl)
         && dyn_cast<CXXMethodDecl>(functionDecl)->isVirtual() ) {
@@ -210,10 +203,19 @@ bool ConstParams::CheckTraverseFunctionDecl(FunctionDecl * functionDecl)
             || name == "Read_And"
             // passed as a LINK<> to another method
             || name == "GlobalBasicErrorHdl_Impl"
+            // template
+            || name == "extract_throw" || name == "readProp"
             )
             return false;
+
     }
 
+    std::string fqn = functionDecl->getQualifiedNameAsString();
+    if ( fqn == "connectivity::jdbc::GlobalRef::set"
+      || fqn == "(anonymous namespace)::ReorderNotifier::operator()"
+      || fqn == "static_txtattr_cast")
+        return false;
+
     // calculate the ones we want to check
     bool foundInterestingParam = false;
     for (const ParmVarDecl *pParmVarDecl : functionDecl->parameters()) {
@@ -222,11 +224,8 @@ bool ConstParams::CheckTraverseFunctionDecl(FunctionDecl * functionDecl)
             || pParmVarDecl->hasAttr<UnusedAttr>())
             continue;
         auto const type = loplugin::TypeCheck(pParmVarDecl->getType());
-        if (!type.Pointer() && !type.LvalueReference())
-            continue;
-        if (type.Pointer().Const())
-            continue;
-        if (type.LvalueReference().Const())
+        if (!( type.Pointer().NonConst()
+             || type.LvalueReference().NonConst()))
             continue;
         // since we normally can't change typedefs, just ignore them
         if (isa<TypedefType>(pParmVarDecl->getType()))
@@ -240,11 +239,6 @@ bool ConstParams::CheckTraverseFunctionDecl(FunctionDecl * functionDecl)
         // const is meaningless when applied to function pointer types
         if (pParmVarDecl->getType()->isFunctionPointerType())
             continue;
-        // ignore things with template params
-        if (pParmVarDecl->getType()->isInstantiationDependentType())
-            continue;
-        if (functionDecl->getIdentifier() && functionDecl->getName() == "WW8TransCol")
-            pParmVarDecl->getType()->dump();
         interestingParamSet.insert(pParmVarDecl);
         parmToFunction[pParmVarDecl] = functionDecl;
         foundInterestingParam = true;
@@ -281,12 +275,23 @@ bool ConstParams::checkIfCanBeConst(const Stmt* stmt, const ParmVarDecl* parmVar
             {
                 for ( auto cxxCtorInitializer : cxxConstructorDecl->inits())
                 {
-                    if (cxxCtorInitializer->isAnyMemberInitializer() && cxxCtorInitializer->getInit() == stmt)
+                    if ( cxxCtorInitializer->getInit() == stmt)
                     {
-                        // if the member is not pointer or ref to-const, we cannot make the param const
-                        auto fieldDecl = cxxCtorInitializer->getAnyMember();
-                        auto tc = loplugin::TypeCheck(fieldDecl->getType());
-                        return tc.Pointer().Const() || tc.LvalueReference().Const();
+                        if (cxxCtorInitializer->isAnyMemberInitializer())
+                        {
+                            // if the member is not pointer-to-const or ref-to-const or value, we cannot make the param const
+                            auto fieldDecl = cxxCtorInitializer->getAnyMember();
+                            auto tc = loplugin::TypeCheck(fieldDecl->getType());
+                            if (tc.Pointer() || tc.LvalueReference())
+                                return tc.Pointer().Const() || tc.LvalueReference().Const();
+                            else
+                                return true;
+                        }
+                        else
+                        {
+                            // probably base initialiser, but no simple way to look up the relevant constructor decl
+                            return false;
+                        }
                     }
                 }
             }
@@ -392,6 +397,7 @@ bool ConstParams::checkIfCanBeConst(const Stmt* stmt, const ParmVarDecl* parmVar
                 }
             }
         }
+        return false;
     } else if (auto callExpr = dyn_cast<CallExpr>(parent)) {
         QualType functionType = callExpr->getCallee()->getType();
         if (functionType->isFunctionPointerType()) {
@@ -437,6 +443,7 @@ bool ConstParams::checkIfCanBeConst(const Stmt* stmt, const ParmVarDecl* parmVar
                 }
             }
         }
+        return false;
     } else if (auto callExpr = dyn_cast<ObjCMessageExpr>(parent)) {
         if (callExpr->getInstanceReceiver() == stmt) {
             return true;
@@ -526,7 +533,7 @@ bool ConstParams::checkIfCanBeConst(const Stmt* stmt, const ParmVarDecl* parmVar
     } else if (isa<CXXTypeidExpr>(parent)) {
         return true;
     } else if (isa<ParenListExpr>(parent)) {
-        return true;
+        return false; // could be improved, seen in constructors when calling base class constructor
     } else if (isa<CXXUnresolvedConstructExpr>(parent)) {
         return false;
     } else if (isa<UnresolvedMemberExpr>(parent)) {
diff --git a/compilerplugins/clang/test/constparams.cxx b/compilerplugins/clang/test/constparams.cxx
index 9390ce2dec17..2cffd87fd5be 100644
--- a/compilerplugins/clang/test/constparams.cxx
+++ b/compilerplugins/clang/test/constparams.cxx
@@ -12,7 +12,7 @@
 struct Class1
 {
     int const * m_f1;
-    Class1(int * f1) : m_f1(f1) {} // expected-error {{this parameter can be const [loplugin:constparams]}}
+    Class1(int * f1) : m_f1(f1) {} // expected-error {{this parameter can be const Class1::Class1 [loplugin:constparams]}}
 };
 
 struct Class2
@@ -38,7 +38,7 @@ void g() {
     P2 p2;
     p2 = (P2) (f3);
 }
-int const * f1(int * p) { // expected-error {{this parameter can be const [loplugin:constparams]}}
+int const * f1(int * p) { // expected-error {{this parameter can be const f1 [loplugin:constparams]}}
     return p;
 }
 void f4(std::string * p) {