new loplugin:stringadd

look for places where we can replace sequential additions to
OUString/OString with one concatentation (i.e. +) expression, which is
more efficient

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

4 files changed, 425 insertions, 3 deletions

diff --git a/compilerplugins/clang/stringadd.cxx b/compilerplugins/clang/stringadd.cxx
new file mode 100644
index 000000000000..641000e28ecf
--- /dev/null
+++ b/compilerplugins/clang/stringadd.cxx
@@ -0,0 +1,273 @@
+/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*- */
+/*
+ * This file is part of the LibreOffice project.
+ *
+ * This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/.
+ */
+#ifndef LO_CLANG_SHARED_PLUGINS
+
+#include <cassert>
+#include <string>
+#include <iostream>
+#include <unordered_map>
+#include <unordered_set>
+
+#include "plugin.hxx"
+#include "check.hxx"
+#include "clang/AST/CXXInheritance.h"
+#include "clang/AST/StmtVisitor.h"
+
+/**
+    Look for repeated addition to OUString/OString.
+
+    Eg.
+      OUString x = "xxx";
+      x += b;
+
+    which can be simplified to
+        x = "xxx" + b
+
+    which is more efficient, because of the OUStringConcat magic.
+*/
+
+namespace
+{
+class StringAdd : public loplugin::FilteringPlugin<StringAdd>
+{
+public:
+    explicit StringAdd(loplugin::InstantiationData const& data)
+        : FilteringPlugin(data)
+    {
+    }
+
+    bool preRun() override
+    {
+        std::string fn(handler.getMainFileName());
+        loplugin::normalizeDotDotInFilePath(fn);
+        if (fn == SRCDIR "/sal/qa/rtl/oustring/rtl_OUString2.cxx"
+            || fn == SRCDIR "/sal/qa/rtl/strings/test_oustring_concat.cxx"
+            || fn == SRCDIR "/sal/qa/rtl/strings/test_ostring_concat.cxx"
+            || fn == SRCDIR "/sal/qa/OStringBuffer/rtl_OStringBuffer.cxx")
+            return false;
+        // there is an ifdef here, but my check is not working, not sure why
+        if (fn == SRCDIR "/pyuno/source/module/pyuno_runtime.cxx")
+            return false;
+        // TODO the += depends on the result of the preceding assign, so can't merge
+        if (fn == SRCDIR "/editeng/source/misc/svxacorr.cxx")
+            return false;
+        return true;
+    }
+
+    virtual void run() override
+    {
+        if (!preRun())
+            return;
+        TraverseDecl(compiler.getASTContext().getTranslationUnitDecl());
+    }
+
+    bool VisitCompoundStmt(CompoundStmt const*);
+
+private:
+    const VarDecl* findAssignOrAdd(Stmt const*);
+    Expr const* ignore(Expr const*);
+    void checkForCompoundAssign(Stmt const* stmt1, Stmt const* stmt2, VarDecl const* varDecl);
+    std::string getSourceAsString(SourceLocation startLoc, SourceLocation endLoc);
+    bool isSideEffectFree(Expr const*);
+};
+
+bool StringAdd::VisitCompoundStmt(CompoundStmt const* compoundStmt)
+{
+    if (ignoreLocation(compoundStmt))
+        return true;
+
+    auto it = compoundStmt->body_begin();
+    while (true)
+    {
+        if (it == compoundStmt->body_end())
+            break;
+        const VarDecl* foundVar = findAssignOrAdd(*it);
+        // reference types have slightly weird behaviour
+        if (foundVar && !foundVar->getType()->isReferenceType())
+        {
+            auto stmt1 = *it;
+            ++it;
+            if (it == compoundStmt->body_end())
+                break;
+            checkForCompoundAssign(stmt1, *it, foundVar);
+            continue;
+        }
+        else
+            ++it;
+    }
+
+    return true;
+}
+
+const VarDecl* StringAdd::findAssignOrAdd(Stmt const* stmt)
+{
+    if (auto exprCleanup = dyn_cast<ExprWithCleanups>(stmt))
+        stmt = exprCleanup->getSubExpr();
+    if (auto switchCase = dyn_cast<SwitchCase>(stmt))
+        stmt = switchCase->getSubStmt();
+
+    if (auto declStmt = dyn_cast<DeclStmt>(stmt))
+        if (declStmt->isSingleDecl())
+            if (auto varDeclLHS = dyn_cast_or_null<VarDecl>(declStmt->getSingleDecl()))
+            {
+                auto tc = loplugin::TypeCheck(varDeclLHS->getType());
+                if (!tc.Class("OUString").Namespace("rtl").GlobalNamespace()
+                    && !tc.Class("OString").Namespace("rtl").GlobalNamespace())
+                    return nullptr;
+                if (varDeclLHS->getStorageDuration() == SD_Static)
+                    return nullptr;
+                if (!varDeclLHS->hasInit())
+                    return nullptr;
+                if (!isSideEffectFree(varDeclLHS->getInit()))
+                    return nullptr;
+                return varDeclLHS;
+            }
+    if (auto operatorCall = dyn_cast<CXXOperatorCallExpr>(stmt))
+        if (operatorCall->getOperator() == OO_Equal || operatorCall->getOperator() == OO_PlusEqual)
+            if (auto declRefExprLHS = dyn_cast<DeclRefExpr>(ignore(operatorCall->getArg(0))))
+                if (auto varDeclLHS = dyn_cast<VarDecl>(declRefExprLHS->getDecl()))
+                {
+                    auto tc = loplugin::TypeCheck(varDeclLHS->getType());
+                    if (!tc.Class("OUString").Namespace("rtl").GlobalNamespace()
+                        && !tc.Class("OString").Namespace("rtl").GlobalNamespace())
+                        return nullptr;
+                    auto rhs = operatorCall->getArg(1);
+                    if (!isSideEffectFree(rhs))
+                        return nullptr;
+                    return varDeclLHS;
+                }
+    return nullptr;
+}
+
+void StringAdd::checkForCompoundAssign(Stmt const* stmt1, Stmt const* stmt2, VarDecl const* varDecl)
+{
+    // OString additions are frequently wrapped in these
+    if (auto exprCleanup = dyn_cast<ExprWithCleanups>(stmt2))
+        stmt2 = exprCleanup->getSubExpr();
+    if (auto switchCase = dyn_cast<SwitchCase>(stmt2))
+        stmt2 = switchCase->getSubStmt();
+    auto operatorCall = dyn_cast<CXXOperatorCallExpr>(stmt2);
+    if (!operatorCall)
+        return;
+    if (operatorCall->getOperator() != OO_PlusEqual)
+        return;
+    auto declRefExprLHS = dyn_cast<DeclRefExpr>(ignore(operatorCall->getArg(0)));
+    if (!declRefExprLHS)
+        return;
+    if (declRefExprLHS->getDecl() != varDecl)
+        return;
+    auto rhs = operatorCall->getArg(1);
+    if (!isSideEffectFree(rhs))
+        return;
+    // if we cross a #ifdef boundary
+    auto s
+        = getSourceAsString(stmt1->getSourceRange().getBegin(), stmt2->getSourceRange().getEnd());
+    if (s.find("#if") != std::string::npos)
+        return;
+    report(DiagnosticsEngine::Warning, "simplify by merging with the preceding assignment",
+           compat::getBeginLoc(stmt2))
+        << stmt2->getSourceRange();
+}
+
+Expr const* StringAdd::ignore(Expr const* expr)
+{
+    return compat::IgnoreImplicit(compat::IgnoreImplicit(expr)->IgnoreParens());
+}
+
+bool StringAdd::isSideEffectFree(Expr const* expr)
+{
+    expr = ignore(expr);
+    // 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,
+    // the result may be incorrect, and you don't necessarily notice in tests because the order is compiler-dependent.
+    // for example see commit afd743141f7a7dd05914d0872c9afe079f16fe0c where such a refactoring introduced such a bug.
+    // So only consider simple RHS expressions.
+    if (!expr->HasSideEffects(compiler.getASTContext()))
+        return true;
+
+    // check for chained adds which are side-effect free
+    if (auto operatorCall = dyn_cast<CXXOperatorCallExpr>(expr))
+    {
+        auto op = operatorCall->getOperator();
+        if (op == OO_PlusEqual || op == OO_Plus)
+            if (isSideEffectFree(operatorCall->getArg(0))
+                && isSideEffectFree(operatorCall->getArg(1)))
+                return true;
+    }
+
+    if (auto callExpr = dyn_cast<CallExpr>(expr))
+    {
+        // check for calls through OUString::number
+        if (auto calleeMethodDecl = dyn_cast_or_null<CXXMethodDecl>(callExpr->getCalleeDecl()))
+            if (calleeMethodDecl && calleeMethodDecl->getIdentifier()
+                && calleeMethodDecl->getName() == "number")
+            {
+                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())
+            {
+                auto name = calleeFunctionDecl->getName();
+                // check for calls through OUStringToOString
+                if (name == "OUStringToOString" || name == "OStringToOUString")
+                    if (isSideEffectFree(callExpr->getArg(0)))
+                        return true;
+                // check for calls through *ResId
+                if (name.endswith("ResId"))
+                    if (isSideEffectFree(callExpr->getArg(0)))
+                        return true;
+            }
+    }
+
+    // sometimes we have a constructor call on the RHS
+    if (auto constructExpr = dyn_cast<CXXConstructExpr>(expr))
+    {
+        auto dc = loplugin::DeclCheck(constructExpr->getConstructor());
+        if (dc.MemberFunction().Class("OUString") || dc.MemberFunction().Class("OString"))
+            if (constructExpr->getNumArgs() == 0 || isSideEffectFree(constructExpr->getArg(0)))
+                return true;
+        // Expr::HasSideEffects does not like stuff that passes through OUStringLiteral
+        auto dc2 = loplugin::DeclCheck(constructExpr->getConstructor()->getParent());
+        if (dc2.Struct("OUStringLiteral").Namespace("rtl").GlobalNamespace())
+            return true;
+    }
+
+    // when adding literals, we sometimes get this
+    if (auto functionalCastExpr = dyn_cast<CXXFunctionalCastExpr>(expr))
+    {
+        auto tc = loplugin::TypeCheck(functionalCastExpr->getType());
+        if (tc.Struct("OUStringLiteral").Namespace("rtl").GlobalNamespace())
+            return isSideEffectFree(functionalCastExpr->getSubExpr());
+    }
+
+    return false;
+}
+
+std::string StringAdd::getSourceAsString(SourceLocation startLoc, SourceLocation endLoc)
+{
+    SourceManager& SM = compiler.getSourceManager();
+    char const* p1 = SM.getCharacterData(startLoc);
+    char const* p2 = SM.getCharacterData(endLoc);
+    p2 += Lexer::MeasureTokenLength(endLoc, SM, compiler.getLangOpts());
+    // sometimes we get weird results, probably from crossing internal LLVM buffer boundaries
+    if (p2 - p1 < 0)
+        return std::string();
+    return std::string(p1, p2 - p1);
+}
+
+loplugin::Plugin::Registration<StringAdd> stringadd("stringadd");
+}
+
+#endif // LO_CLANG_SHARED_PLUGINS
+
+/* vim:set shiftwidth=4 softtabstop=4 expandtab: */
diff --git a/compilerplugins/clang/test/stringadd.cxx b/compilerplugins/clang/test/stringadd.cxx
new file mode 100644
index 000000000000..c9ef37f09bfd
--- /dev/null
+++ b/compilerplugins/clang/test/stringadd.cxx
@@ -0,0 +1,150 @@
+/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4; fill-column: 100 -*- */
+/*
+ * This file is part of the LibreOffice project.
+ *
+ * This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/.
+ */
+
+#include <rtl/ustring.hxx>
+#include <rtl/string.hxx>
+
+namespace test1
+{
+static const char XXX1[] = "xxx";
+static const char XXX2[] = "xxx";
+void f(OUString s1, int i, OString o)
+{
+    OUString s2 = s1;
+    // expected-error@+1 {{simplify by merging with the preceding assignment [loplugin:stringadd]}}
+    s2 += "xxx";
+    // expected-error@+1 {{simplify by merging with the preceding assignment [loplugin:stringadd]}}
+    s2 += "xxx";
+    // expected-error@+1 {{simplify by merging with the preceding assignment [loplugin:stringadd]}}
+    s2 += s1;
+    s2 = s1 + "xxx";
+    // expected-error@+1 {{simplify by merging with the preceding assignment [loplugin:stringadd]}}
+    s2 += s1;
+    // expected-error@+1 {{simplify by merging with the preceding assignment [loplugin:stringadd]}}
+    s2 += OUString::number(i);
+    // expected-error@+1 {{simplify by merging with the preceding assignment [loplugin:stringadd]}}
+    s2 += XXX1;
+    // expected-error@+1 {{simplify by merging with the preceding assignment [loplugin:stringadd]}}
+    s2 += OUStringLiteral(XXX1) + XXX2;
+
+    // expected-error@+1 {{simplify by merging with the preceding assignment [loplugin:stringadd]}}
+    s2 += OStringToOUString(o, RTL_TEXTENCODING_UTF8);
+}
+void g(OString s1, int i, OUString u)
+{
+    OString s2 = s1;
+    // expected-error@+1 {{simplify by merging with the preceding assignment [loplugin:stringadd]}}
+    s2 += "xxx";
+    // expected-error@+1 {{simplify by merging with the preceding assignment [loplugin:stringadd]}}
+    s2 += "xxx";
+    // expected-error@+1 {{simplify by merging with the preceding assignment [loplugin:stringadd]}}
+    s2 += s1;
+    s2 = s1 + "xxx";
+    // expected-error@+1 {{simplify by merging with the preceding assignment [loplugin:stringadd]}}
+    s2 += s1;
+    // expected-error@+1 {{simplify by merging with the preceding assignment [loplugin:stringadd]}}
+    s2 += OString::number(i);
+
+    // expected-error@+1 {{simplify by merging with the preceding assignment [loplugin:stringadd]}}
+    s2 += OUStringToOString(u, RTL_TEXTENCODING_ASCII_US);
+}
+}
+
+namespace test2
+{
+void f(OUString s3)
+{
+    s3 += "xxx";
+    // expected-error@+1 {{simplify by merging with the preceding assignment [loplugin:stringadd]}}
+    s3 += "xxx";
+}
+void g(OString s3)
+{
+    s3 += "xxx";
+    // expected-error@+1 {{simplify by merging with the preceding assignment [loplugin:stringadd]}}
+    s3 += "xxx";
+}
+}
+
+namespace test3
+{
+struct Bar
+{
+    OUString m_field;
+};
+void f(Bar b1, Bar& b2, Bar* b3)
+{
+    OUString s3 = "xxx";
+    // expected-error@+1 {{simplify by merging with the preceding assignment [loplugin:stringadd]}}
+    s3 += b1.m_field;
+    // expected-error@+1 {{simplify by merging with the preceding assignment [loplugin:stringadd]}}
+    s3 += b2.m_field;
+    // expected-error@+1 {{simplify by merging with the preceding assignment [loplugin:stringadd]}}
+    s3 += b3->m_field;
+}
+}
+
+// no warning expected
+namespace test4
+{
+void f()
+{
+    OUString sRet = "xxx";
+#if OSL_DEBUG_LEVEL > 0
+    sRet += ";";
+#endif
+}
+}
+
+// no warning expected
+namespace test5
+{
+OUString side_effect();
+int side_effect2();
+void f()
+{
+    OUString sRet = "xxx";
+    sRet += side_effect();
+    sRet += OUString::number(side_effect2());
+}
+void g()
+{
+    OUString sRet = side_effect();
+    sRet += "xxx";
+}
+}
+
+namespace test6
+{
+void f(OUString sComma, OUString maExtension, int mnDocumentIconID)
+{
+    OUString sValue;
+    // expected-error@+1 {{simplify by merging with the preceding assignment [loplugin:stringadd]}}
+    sValue += sComma + sComma + maExtension + sComma;
+    // expected-error@+1 {{simplify by merging with the preceding assignment [loplugin:stringadd]}}
+    sValue += OUString::number(mnDocumentIconID) + sComma;
+}
+struct Foo
+{
+    OUString sFormula1;
+};
+void g(int x, const Foo& aValidation)
+{
+    OUString sCondition;
+    switch (x)
+    {
+        case 1:
+            sCondition += "cell-content-is-in-list(";
+            // expected-error@+1 {{simplify by merging with the preceding assignment [loplugin:stringadd]}}
+            sCondition += aValidation.sFormula1 + ")";
+    }
+}
+}
+
+/* vim:set shiftwidth=4 softtabstop=4 expandtab cinoptions=b1,g0,N-s cinkeys+=0=break: */
diff --git a/solenv/CompilerTest_compilerplugins_clang.mk b/solenv/CompilerTest_compilerplugins_clang.mk
index b76d3767149b..9a5fe8545ee3 100644
--- a/solenv/CompilerTest_compilerplugins_clang.mk
+++ b/solenv/CompilerTest_compilerplugins_clang.mk
@@ -70,6 +70,7 @@ $(eval $(call gb_CompilerTest_add_exception_objects,compilerplugins_clang, \
     compilerplugins/clang/test/staticconstfield \
     compilerplugins/clang/test/staticvar \
     compilerplugins/clang/test/stdfunction \
+    compilerplugins/clang/test/stringadd \
     compilerplugins/clang/test/stringbuffer \
     compilerplugins/clang/test/stringconcatauto \
     compilerplugins/clang/test/stringconcatliterals \
diff --git a/vbahelper/source/vbahelper/vbacommandbarhelper.cxx b/vbahelper/source/vbahelper/vbacommandbarhelper.cxx
index 014e04035be9..715923e92303 100644
--- a/vbahelper/source/vbahelper/vbacommandbarhelper.cxx
+++ b/vbahelper/source/vbahelper/vbacommandbarhelper.cxx
@@ -238,9 +238,7 @@ sal_Int32 VbaCommandBarHelper::findControlByName( const css::uno::Reference< css
 
 OUString VbaCommandBarHelper::generateCustomURL()
 {
-    OUString url( ITEM_TOOLBAR_URL );
-    url += CUSTOM_TOOLBAR_STR;
-
+    OUString url = OUStringLiteral(ITEM_TOOLBAR_URL) + CUSTOM_TOOLBAR_STR;
     // use a random number to minimize possible clash with existing custom toolbars
     url += OUString::number(comphelper::rng::uniform_int_distribution(0, std::numeric_limits<int>::max()), 16);
     return url;