loplugin:constvars, look for loop vars that can be const

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

2 files changed, 117 insertions, 13 deletions

diff --git a/compilerplugins/clang/constvars.cxx b/compilerplugins/clang/constvars.cxx
index 56f863407f0b..d4a431dc14f0 100644
--- a/compilerplugins/clang/constvars.cxx
+++ b/compilerplugins/clang/constvars.cxx
@@ -127,6 +127,7 @@ public:
     bool shouldVisitImplicitCode() const { return true; }
 
     bool VisitVarDecl(const VarDecl*);
+    bool VisitCXXForRangeStmt(const CXXForRangeStmt*);
     bool VisitDeclRefExpr(const DeclRefExpr*);
     bool TraverseCXXConstructorDecl(CXXConstructorDecl*);
     bool TraverseCXXMethodDecl(CXXMethodDecl*);
@@ -167,7 +168,7 @@ void ConstVars::run()
         // Implement a marker that disables this plugins warning at a specific site
         if (sourceString.contains("loplugin:constvars:ignore"))
             continue;
-        report(DiagnosticsEngine::Warning, "static var can be const", compat::getBeginLoc(v));
+        report(DiagnosticsEngine::Warning, "var can be const", compat::getBeginLoc(v));
     }
 }
 
@@ -178,6 +179,8 @@ bool ConstVars::VisitVarDecl(const VarDecl* varDecl)
         return true;
     if (!varDecl->hasGlobalStorage())
         return true;
+    if (isa<ParmVarDecl>(varDecl))
+        return true;
     if (varDecl->getLinkageAndVisibility().getLinkage() == ExternalLinkage)
         return true;
     if (varDecl->getType().isConstQualified())
@@ -192,6 +195,8 @@ bool ConstVars::VisitVarDecl(const VarDecl* varDecl)
 
     if (!varDecl->getInit())
         return true;
+    if (varDecl->getInit()->isInstantiationDependent())
+        return true;
     if (!varDecl->getInit()->isCXX11ConstantExpr(compiler.getASTContext()))
         return true;
 
@@ -199,6 +204,26 @@ bool ConstVars::VisitVarDecl(const VarDecl* varDecl)
     return true;
 }
 
+bool ConstVars::VisitCXXForRangeStmt(const CXXForRangeStmt* forStmt)
+{
+    if (compat::getBeginLoc(forStmt).isValid() && ignoreLocation(forStmt))
+        return true;
+    const VarDecl* varDecl = forStmt->getLoopVariable();
+    if (!varDecl)
+        return true;
+    // we don't handle structured assignment properly
+    if (isa<DecompositionDecl>(varDecl))
+        return true;
+    auto tc = loplugin::TypeCheck(varDecl->getType());
+    if (!tc.LvalueReference())
+        return true;
+    if (tc.LvalueReference().Const())
+        return true;
+
+    definitionSet.insert(varDecl);
+    return true;
+}
+
 bool ConstVars::TraverseCXXConstructorDecl(CXXConstructorDecl* cxxConstructorDecl)
 {
     auto copy = insideMoveOrCopyDeclParent;
@@ -276,7 +301,9 @@ void ConstVars::check(const VarDecl* varDecl, const Expr* memberExpr)
     const Stmt* child = memberExpr;
     const Stmt* parent
         = parentsRange.begin() == parentsRange.end() ? nullptr : parentsRange.begin()->get<Stmt>();
+
     // walk up the tree until we find something interesting
+
     bool bCannotBeConst = false;
     bool bDump = false;
     auto walkUp = [&]() {
@@ -295,16 +322,25 @@ void ConstVars::check(const VarDecl* varDecl, const Expr* memberExpr)
             if (parentsRange.begin() != parentsRange.end())
             {
                 auto varDecl = dyn_cast_or_null<VarDecl>(parentsRange.begin()->get<Decl>());
-                // The isImplicit() call is to avoid triggering when we see the vardecl which is part of a for-range statement,
-                // which is of type 'T&&' and also an l-value-ref ?
-                if (varDecl && !varDecl->isImplicit()
-                    && loplugin::TypeCheck(varDecl->getType()).LvalueReference().NonConst())
+                if (varDecl)
                 {
-                    bCannotBeConst = true;
+                    if (varDecl->isImplicit())
+                    {
+                        // so we can walk up from inside a for-range stmt
+                        parentsRange = compiler.getASTContext().getParents(*varDecl);
+                        if (parentsRange.begin() != parentsRange.end())
+                            parent = parentsRange.begin()->get<Stmt>();
+                    }
+                    else if (loplugin::TypeCheck(varDecl->getType()).LvalueReference().NonConst())
+                    {
+                        bCannotBeConst = true;
+                        break;
+                    }
                 }
             }
-            break;
         }
+        if (!parent)
+            break;
         if (isa<CXXReinterpretCastExpr>(parent))
         {
             // once we see one of these, there is not much useful we can know
@@ -422,9 +458,18 @@ void ConstVars::check(const VarDecl* varDecl, const Expr* memberExpr)
             }
             break;
         }
+        else if (auto rangeStmt = dyn_cast<CXXForRangeStmt>(parent))
+        {
+            if (rangeStmt->getRangeStmt() == child)
+            {
+                auto tc = loplugin::TypeCheck(rangeStmt->getLoopVariable()->getType());
+                if (tc.LvalueReference().NonConst())
+                    bCannotBeConst = true;
+            }
+            break;
+        }
         else if (isa<SwitchStmt>(parent) || isa<WhileStmt>(parent) || isa<ForStmt>(parent)
-                 || isa<IfStmt>(parent) || isa<DoStmt>(parent) || isa<CXXForRangeStmt>(parent)
-                 || isa<DefaultStmt>(parent))
+                 || isa<IfStmt>(parent) || isa<DoStmt>(parent) || isa<DefaultStmt>(parent))
         {
             break;
         }
@@ -470,10 +515,11 @@ bool ConstVars::IsPassedByNonConst(const VarDecl* varDecl, const Stmt* child,
     {
         for (unsigned i = 0; i < len; ++i)
             if (callExpr.getArg(i) == child)
-                if (loplugin::TypeCheck(calleeFunctionDecl.getParamType(i))
-                        .LvalueReference()
-                        .NonConst())
+            {
+                auto tc = loplugin::TypeCheck(calleeFunctionDecl.getParamType(i));
+                if (tc.LvalueReference().NonConst() || tc.Pointer().NonConst())
                     return true;
+            }
     }
     return false;
 }
diff --git a/compilerplugins/clang/test/constvars.cxx b/compilerplugins/clang/test/constvars.cxx
index 40f3250a6512..dc3c1ecb9c6b 100644
--- a/compilerplugins/clang/test/constvars.cxx
+++ b/compilerplugins/clang/test/constvars.cxx
@@ -12,11 +12,17 @@
 #else
 
 #include <com/sun/star/uno/Any.hxx>
+#include <com/sun/star/uno/Sequence.hxx>
+#include <com/sun/star/uno/XInterface.hpp>
+#include <map>
+#include <list>
+#include <vector>
+#include <rtl/ustring.hxx>
 
 namespace test1
 {
 int const aFormalArgs[] = { 1, 2 };
-// expected-error@+1 {{static var can be const [loplugin:constvars]}}
+// expected-error@+1 {{var can be const [loplugin:constvars]}}
 static sal_uInt16 nMediaArgsCount = SAL_N_ELEMENTS(aFormalArgs);
 sal_uInt16 foo()
 {
@@ -44,6 +50,58 @@ static sal_uInt16 nMediaArgsCount = 1; // loplugin:constvars:ignore
 sal_uInt16 foo() { return nMediaArgsCount; }
 };
 
+// no warning expected, we don't handle these destructuring assignments properly yet
+namespace test4
+{
+void foo()
+{
+    std::map<OUString, OUString> aMap;
+    for (auto & [ rName, rEntry ] : aMap)
+    {
+        rEntry.clear();
+    }
+}
+};
+
+// no warning expected
+namespace test5
+{
+struct Struct1
+{
+};
+void release(Struct1*);
+void foo(std::list<Struct1*> aList)
+{
+    for (Struct1* pItem : aList)
+    {
+        release(pItem);
+    }
+}
+};
+
+namespace test6
+{
+void foo(css::uno::Sequence<css::uno::Reference<css::uno::XInterface>>& aSeq)
+{
+    // expected-error@+1 {{var can be const [loplugin:constvars]}}
+    for (css::uno::Reference<css::uno::XInterface>& x : aSeq)
+    {
+        x.get();
+    }
+}
+};
+
+// no warning expected
+namespace test7
+{
+void foo(std::vector<std::vector<int>> aVecVec)
+{
+    for (auto& rVec : aVecVec)
+        for (auto& rElement : rVec)
+            rElement = 1;
+}
+};
+
 #endif
 
 /* vim:set shiftwidth=4 softtabstop=4 expandtab cinoptions=b1,g0,N-s cinkeys+=0=break: */