Fix StringAdd::isCompileTimeConstant

...to find StringLiteral on the RHS of +=.  Which revealed that the
VisitCompoundStmt/checkForCompoundAssign logic needed to be fixed, too, so that

  s += side_effect();
  s += "literal";
  s += side_effect();

only gets combined to

  s += side_effect() + "literal";
  s += side_effect();

and not all the way to

  s += side_effect() + "literal" + side_effect();

Change-Id: I432e3458b933a7d0ad6141c747b675cc8b0f0ba4
Reviewed-on: https://gerrit.libreoffice.org/81804
Tested-by: Jenkins
Reviewed-by: Stephan Bergmann <sbergman@redhat.com>

2 files changed, 84 insertions, 35 deletions

diff --git a/compilerplugins/clang/stringadd.cxx b/compilerplugins/clang/stringadd.cxx
index 653c281f0ac7..e9df02bd10c0 100644
--- a/compilerplugins/clang/stringadd.cxx
+++ b/compilerplugins/clang/stringadd.cxx
@@ -74,16 +74,21 @@ public:
     bool VisitCXXOperatorCallExpr(CXXOperatorCallExpr const*);
 
 private:
-    struct VarDeclAndConstant
+    enum class Summands
+    {
+        OnlyCompileTimeConstants,
+        OnlySideEffectFree,
+        SideEffect
+    };
+
+    struct VarDeclAndSummands
     {
         const VarDecl* varDecl;
-        bool bCompileTimeConstant;
-        bool bSideEffectFree;
+        Summands summands;
     };
 
-    VarDeclAndConstant findAssignOrAdd(Stmt const*);
-    void checkForCompoundAssign(Stmt const* stmt1, Stmt const* stmt2,
-                                VarDeclAndConstant const& varDecl);
+    VarDeclAndSummands findAssignOrAdd(Stmt const*);
+    bool checkForCompoundAssign(Stmt const* stmt1, Stmt const* stmt2, VarDeclAndSummands& varDecl);
 
     Expr const* ignore(Expr const*);
     bool isSideEffectFree(Expr const*);
@@ -100,16 +105,21 @@ bool StringAdd::VisitCompoundStmt(CompoundStmt const* compoundStmt)
     {
         if (it == compoundStmt->body_end())
             break;
-        VarDeclAndConstant foundVar = findAssignOrAdd(*it);
+        VarDeclAndSummands foundVar = findAssignOrAdd(*it);
         // reference types have slightly weird behaviour
         if (foundVar.varDecl && !foundVar.varDecl->getType()->isReferenceType())
         {
             auto stmt1 = *it;
             ++it;
-            if (it == compoundStmt->body_end())
-                break;
-            checkForCompoundAssign(stmt1, *it, foundVar);
-            continue;
+            while (it != compoundStmt->body_end())
+            {
+                if (!checkForCompoundAssign(stmt1, *it, foundVar))
+                {
+                    break;
+                }
+                stmt1 = *it;
+                ++it;
+            }
         }
         else
             ++it;
@@ -118,7 +128,7 @@ bool StringAdd::VisitCompoundStmt(CompoundStmt const* compoundStmt)
     return true;
 }
 
-StringAdd::VarDeclAndConstant StringAdd::findAssignOrAdd(Stmt const* stmt)
+StringAdd::VarDeclAndSummands StringAdd::findAssignOrAdd(Stmt const* stmt)
 {
     if (auto exprCleanup = dyn_cast<ExprWithCleanups>(stmt))
         stmt = exprCleanup->getSubExpr();
@@ -137,8 +147,11 @@ StringAdd::VarDeclAndConstant StringAdd::findAssignOrAdd(Stmt const* stmt)
                     return {};
                 if (!varDeclLHS->hasInit())
                     return {};
-                return { varDeclLHS, isCompileTimeConstant(varDeclLHS->getInit()),
-                         isSideEffectFree(varDeclLHS->getInit()) };
+                return { varDeclLHS, (isCompileTimeConstant(varDeclLHS->getInit())
+                                          ? Summands::OnlyCompileTimeConstants
+                                          : (isSideEffectFree(varDeclLHS->getInit())
+                                                 ? Summands::OnlySideEffectFree
+                                                 : Summands::SideEffect)) };
             }
     if (auto operatorCall = dyn_cast<CXXOperatorCallExpr>(stmt))
         if (operatorCall->getOperator() == OO_Equal || operatorCall->getOperator() == OO_PlusEqual)
@@ -150,13 +163,17 @@ StringAdd::VarDeclAndConstant StringAdd::findAssignOrAdd(Stmt const* stmt)
                         && !tc.Class("OString").Namespace("rtl").GlobalNamespace())
                         return {};
                     auto rhs = operatorCall->getArg(1);
-                    return { varDeclLHS, isCompileTimeConstant(rhs), isSideEffectFree(rhs) };
+                    return { varDeclLHS,
+                             (isCompileTimeConstant(rhs)
+                                  ? Summands::OnlyCompileTimeConstants
+                                  : (isSideEffectFree(rhs) ? Summands::OnlySideEffectFree
+                                                           : Summands::SideEffect)) };
                 }
     return {};
 }
 
-void StringAdd::checkForCompoundAssign(Stmt const* stmt1, Stmt const* stmt2,
-                                       VarDeclAndConstant const& varDecl)
+bool StringAdd::checkForCompoundAssign(Stmt const* stmt1, Stmt const* stmt2,
+                                       VarDeclAndSummands& varDecl)
 {
     // OString additions are frequently wrapped in these
     if (auto exprCleanup = dyn_cast<ExprWithCleanups>(stmt2))
@@ -165,28 +182,42 @@ void StringAdd::checkForCompoundAssign(Stmt const* stmt1, Stmt const* stmt2,
         stmt2 = switchCase->getSubStmt();
     auto operatorCall = dyn_cast<CXXOperatorCallExpr>(stmt2);
     if (!operatorCall)
-        return;
+        return false;
     if (operatorCall->getOperator() != OO_PlusEqual)
-        return;
+        return false;
     auto declRefExprLHS = dyn_cast<DeclRefExpr>(ignore(operatorCall->getArg(0)));
     if (!declRefExprLHS)
-        return;
+        return false;
     if (declRefExprLHS->getDecl() != varDecl.varDecl)
-        return;
+        return false;
     // if either side is a compile-time-constant, then we don't care about
     // side-effects
     auto rhs = operatorCall->getArg(1);
-    if (varDecl.bCompileTimeConstant || isCompileTimeConstant(rhs))
-        ; // good
-    else if (!varDecl.bSideEffectFree || !isSideEffectFree(rhs))
-        return;
+    auto const ctcRhs = isCompileTimeConstant(rhs);
+    if (!ctcRhs)
+    {
+        auto const sefRhs = isSideEffectFree(rhs);
+        auto const oldSummands = varDecl.summands;
+        varDecl.summands = sefRhs ? Summands::OnlySideEffectFree : Summands::SideEffect;
+        if (oldSummands != Summands::OnlyCompileTimeConstants
+            && (oldSummands == Summands::SideEffect || !sefRhs))
+        {
+            return true;
+        }
+    }
     // if we cross a #ifdef boundary
     if (containsPreprocessingConditionalInclusion(
             SourceRange(stmt1->getSourceRange().getBegin(), stmt2->getSourceRange().getEnd())))
-        return;
+    {
+        varDecl.summands
+            = ctcRhs ? Summands::OnlyCompileTimeConstants
+                     : isSideEffectFree(rhs) ? Summands::OnlySideEffectFree : Summands::SideEffect;
+        return true;
+    }
     report(DiagnosticsEngine::Warning, "simplify by merging with the preceding assignment",
            compat::getBeginLoc(stmt2))
         << stmt2->getSourceRange();
+    return true;
 }
 
 // Check for generating temporaries when adding strings
@@ -318,10 +349,9 @@ bool StringAdd::isCompileTimeConstant(Expr const* expr)
 {
     expr = compat::IgnoreImplicit(expr);
     if (auto cxxConstructExpr = dyn_cast<CXXConstructExpr>(expr))
-        if (cxxConstructExpr->getNumArgs() > 0
-            && isa<clang::StringLiteral>(cxxConstructExpr->getArg(0)))
-            return true;
-    return false;
+        if (cxxConstructExpr->getNumArgs() > 0)
+            expr = cxxConstructExpr->getArg(0);
+    return isa<clang::StringLiteral>(expr);
 }
 
 loplugin::Plugin::Registration<StringAdd> stringadd("stringadd");
diff --git a/compilerplugins/clang/test/stringadd.cxx b/compilerplugins/clang/test/stringadd.cxx
index 00582f2db459..748ee35cfe61 100644
--- a/compilerplugins/clang/test/stringadd.cxx
+++ b/compilerplugins/clang/test/stringadd.cxx
@@ -65,16 +65,21 @@ void f3(OUString aStr, int nFirstContent)
     // expected-error@+1 {{simplify by merging with the preceding assignment [loplugin:stringadd]}}
     aFirstStr += "...";
 }
+OUString side_effect();
 void f4(int i)
 {
-    OUString s("xxx");
+    OUString s1;
+    OUString s2("xxx");
     // expected-error@+1 {{simplify by merging with the preceding assignment [loplugin:stringadd]}}
-    s += "xxx";
+    s2 += "xxx";
     ++i;
     // any other kind of statement breaks the chain (at least for now)
-    s += "xxx";
+    s2 += "xxx";
     // expected-error@+1 {{simplify by merging with the preceding assignment [loplugin:stringadd]}}
-    s += "xxx";
+    s2 += side_effect();
+    s1 += "yyy";
+    // expected-error@+1 {{simplify by merging with the preceding assignment [loplugin:stringadd]}}
+    s1 += "yyy";
 }
 }
 
@@ -111,23 +116,37 @@ void f(Bar b1, Bar& b2, Bar* b3)
     s3 += b3->m_field;
 }
 OUString side_effect();
-void f2()
+void f2(OUString s)
 {
     OUString sRet = "xxx";
     // expected-error@+1 {{simplify by merging with the preceding assignment [loplugin:stringadd]}}
     sRet += side_effect();
+    // expected-error@+1 {{simplify by merging with the preceding assignment [loplugin:stringadd]}}
+    sRet += "xxx";
+    sRet += side_effect();
+    // expected-error@+1 {{simplify by merging with the preceding assignment [loplugin:stringadd]}}
+    sRet += "xxx";
+    // expected-error@+1 {{simplify by merging with the preceding assignment [loplugin:stringadd]}}
+    sRet += "xxx";
+    sRet += s;
+    // expected-error@+1 {{simplify by merging with the preceding assignment [loplugin:stringadd]}}
+    sRet += "xxx";
 }
 }
 
 // no warning expected
 namespace test4
 {
+OUString side_effect();
 void f()
 {
     OUString sRet = "xxx";
 #if OSL_DEBUG_LEVEL > 0
     sRet += ";";
 #endif
+    sRet += " ";
+    // expected-error@+1 {{simplify by merging with the preceding assignment [loplugin:stringadd]}}
+    sRet += side_effect();
 }
 }