loplugin:stringadd look for unnecessary temporaries

which defeat the *StringConcat optimisation.
Also make StringConcat conversions treat a nullptr as an empty string,
to match the O*String(char*) constructors.

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

2 files changed, 122 insertions, 11 deletions

diff --git a/compilerplugins/clang/stringadd.cxx b/compilerplugins/clang/stringadd.cxx
index 641000e28ecf..a3435bab7cb5 100644
--- a/compilerplugins/clang/stringadd.cxx
+++ b/compilerplugins/clang/stringadd.cxx
@@ -68,11 +68,13 @@ public:
     }
 
     bool VisitCompoundStmt(CompoundStmt const*);
+    bool VisitCXXOperatorCallExpr(CXXOperatorCallExpr const*);
 
 private:
     const VarDecl* findAssignOrAdd(Stmt const*);
-    Expr const* ignore(Expr const*);
     void checkForCompoundAssign(Stmt const* stmt1, Stmt const* stmt2, VarDecl const* varDecl);
+
+    Expr const* ignore(Expr const*);
     std::string getSourceAsString(SourceLocation startLoc, SourceLocation endLoc);
     bool isSideEffectFree(Expr const*);
 };
@@ -175,6 +177,65 @@ void StringAdd::checkForCompoundAssign(Stmt const* stmt1, Stmt const* stmt2, Var
         << stmt2->getSourceRange();
 }
 
+// Check for generating temporaries when adding strings
+//
+bool StringAdd::VisitCXXOperatorCallExpr(CXXOperatorCallExpr const* operatorCall)
+{
+    if (ignoreLocation(operatorCall))
+        return true;
+    // TODO PlusEqual seems to generate temporaries, does not do the StringConcat optimisation
+    if (operatorCall->getOperator() != OO_Plus)
+        return true;
+    auto tc = loplugin::TypeCheck(operatorCall->getType()->getUnqualifiedDesugaredType());
+    if (!tc.Struct("OUStringConcat").Namespace("rtl").GlobalNamespace()
+        && !tc.Struct("OStringConcat").Namespace("rtl").GlobalNamespace()
+        && !tc.Class("OUString").Namespace("rtl").GlobalNamespace()
+        && !tc.Class("OString").Namespace("rtl").GlobalNamespace())
+        return true;
+
+    auto check = [operatorCall, this, &tc](const MaterializeTemporaryExpr* matTempExpr) {
+        auto tc3 = loplugin::TypeCheck(matTempExpr->getType());
+        if (!tc3.Class("OUString").Namespace("rtl").GlobalNamespace()
+            && !tc3.Class("OString").Namespace("rtl").GlobalNamespace())
+            return;
+        if (auto bindTemp
+            = dyn_cast<CXXBindTemporaryExpr>(matTempExpr->GetTemporaryExpr()->IgnoreCasts()))
+        {
+            // ignore temporaries returned from function calls
+            if (isa<CallExpr>(bindTemp->getSubExpr()))
+                return;
+            // we don't have OStringLiteral1, so char needs to generate a temporary
+            if (tc.Class("OString").Namespace("rtl").GlobalNamespace()
+                || tc.Struct("OStringConcat").Namespace("rtl").GlobalNamespace())
+                if (auto cxxConstruct = dyn_cast<CXXConstructExpr>(bindTemp->getSubExpr()))
+                    if (loplugin::TypeCheck(
+                            cxxConstruct->getConstructor()->getParamDecl(0)->getType())
+                            .Char())
+                        return;
+            // calls where we pass in an explicit character encoding
+            if (auto cxxTemp = dyn_cast<CXXTemporaryObjectExpr>(bindTemp->getSubExpr()))
+                if (cxxTemp->getNumArgs() > 1)
+                    return;
+        }
+        // conditional operators ( a ? b : c ) will result in temporaries
+        if (isa<ConditionalOperator>(
+                matTempExpr->GetTemporaryExpr()->IgnoreCasts()->IgnoreParens()))
+            return;
+        report(DiagnosticsEngine::Warning, "avoid constructing temporary copies during +",
+               compat::getBeginLoc(matTempExpr))
+            << matTempExpr->getSourceRange();
+        //        operatorCall->dump();
+        //        matTempExpr->getType()->dump();
+        //        operatorCall->getType()->getUnqualifiedDesugaredType()->dump();
+    };
+
+    if (auto matTempExpr = dyn_cast<MaterializeTemporaryExpr>(operatorCall->getArg(0)))
+        check(matTempExpr);
+    if (auto matTempExpr = dyn_cast<MaterializeTemporaryExpr>(operatorCall->getArg(1)))
+        check(matTempExpr);
+    return true;
+}
+
 Expr const* StringAdd::ignore(Expr const* expr)
 {
     return compat::IgnoreImplicit(compat::IgnoreImplicit(expr)->IgnoreParens());
@@ -183,6 +244,9 @@ Expr const* StringAdd::ignore(Expr const* expr)
 bool StringAdd::isSideEffectFree(Expr const* expr)
 {
     expr = ignore(expr);
+    // I don't think the OUStringAppend functionality can handle this efficiently
+    if (isa<ConditionalOperator>(expr))
+        return false;
     // Multiple statements have a well defined evaluation order (sequence points between them)
     // but a single expression may be evaluated in arbitrary order;
     // if there are side effects in one of the sub-expressions that have an effect on another subexpression,
@@ -204,15 +268,20 @@ bool StringAdd::isSideEffectFree(Expr const* expr)
 
     if (auto callExpr = dyn_cast<CallExpr>(expr))
     {
-        // check for calls through OUString::number
+        // check for calls through OUString::number/OUString::unacquired
         if (auto calleeMethodDecl = dyn_cast_or_null<CXXMethodDecl>(callExpr->getCalleeDecl()))
-            if (calleeMethodDecl && calleeMethodDecl->getIdentifier()
-                && calleeMethodDecl->getName() == "number")
+            if (calleeMethodDecl && calleeMethodDecl->getIdentifier())
             {
-                auto tc = loplugin::TypeCheck(calleeMethodDecl->getParent());
-                if (tc.Class("OUString") || tc.Class("OString"))
-                    if (isSideEffectFree(callExpr->getArg(0)))
-                        return true;
+                auto name = calleeMethodDecl->getName();
+                if (name == "number" || name == "unacquired")
+                {
+                    auto tc = loplugin::TypeCheck(calleeMethodDecl->getParent());
+                    if (tc.Class("OUString") || tc.Class("OString"))
+                    {
+                        if (isSideEffectFree(callExpr->getArg(0)))
+                            return true;
+                    }
+                }
             }
         if (auto calleeFunctionDecl = dyn_cast_or_null<FunctionDecl>(callExpr->getCalleeDecl()))
             if (calleeFunctionDecl && calleeFunctionDecl->getIdentifier())
@@ -222,8 +291,8 @@ bool StringAdd::isSideEffectFree(Expr const* expr)
                 if (name == "OUStringToOString" || name == "OStringToOUString")
                     if (isSideEffectFree(callExpr->getArg(0)))
                         return true;
-                // check for calls through *ResId
-                if (name.endswith("ResId"))
+                // whitelist some known-safe methods
+                if (name.endswith("ResId") || name == "GetXMLToken")
                     if (isSideEffectFree(callExpr->getArg(0)))
                         return true;
             }
diff --git a/compilerplugins/clang/test/stringadd.cxx b/compilerplugins/clang/test/stringadd.cxx
index c9ef37f09bfd..0e0986258254 100644
--- a/compilerplugins/clang/test/stringadd.cxx
+++ b/compilerplugins/clang/test/stringadd.cxx
@@ -7,8 +7,13 @@
  * file, You can obtain one at http://mozilla.org/MPL/2.0/.
  */
 
-#include <rtl/ustring.hxx>
+#include <rtl/strbuf.hxx>
 #include <rtl/string.hxx>
+#include <rtl/ustrbuf.hxx>
+#include <rtl/ustring.hxx>
+
+// ---------------------------------------------------------------
+// += tests
 
 namespace test1
 {
@@ -147,4 +152,41 @@ void g(int x, const Foo& aValidation)
 }
 }
 
+// ---------------------------------------------------------------
+// detecting OUString temporary construction in +
+
+namespace test9
+{
+OUString getByValue();
+const OUString& getByRef();
+void f1(OUString s, OUString t, int i, const char* pChar)
+{
+    // no warning expected
+    t = t + "xxx";
+    // expected-error@+1 {{avoid constructing temporary copies during + [loplugin:stringadd]}}
+    s = s + OUString("xxx");
+    // expected-error@+1 {{avoid constructing temporary copies during + [loplugin:stringadd]}}
+    s = s + OUString(getByRef());
+
+    // no warning expected
+    OUString a;
+    a = a + getByValue();
+
+    // no warning expected
+    OUString b;
+    b = b + (i == 1 ? "aaa" : "bbb");
+
+    // no warning expected
+    OUString c;
+    c = c + OUString(pChar, strlen(pChar), RTL_TEXTENCODING_UTF8);
+}
+void f2(char ch)
+{
+    OString s;
+    // expected-error@+1 {{avoid constructing temporary copies during + [loplugin:stringadd]}}
+    s = s + OString("xxx");
+    // no warning expected, no OStringLiteral1
+    s = s + OString(ch);
+}
+}
 /* vim:set shiftwidth=4 softtabstop=4 expandtab cinoptions=b1,g0,N-s cinkeys+=0=break: */