loplugin:stringadd convert chained append to +

which can use the more efficient *StringConcat

Also fix a crash in stringview plugin which
started happening while I working on this.

Change-Id: I91a5b9b7707d1594d27d80b73930f5afac8ae608
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/114568
Tested-by: Jenkins
Reviewed-by: Noel Grandin <noel.grandin@collabora.co.uk>

3 files changed, 63 insertions, 1 deletions

diff --git a/compilerplugins/clang/stringadd.cxx b/compilerplugins/clang/stringadd.cxx
index 5723b5bb6e3b..9c6d11fd55b1 100644
--- a/compilerplugins/clang/stringadd.cxx
+++ b/compilerplugins/clang/stringadd.cxx
@@ -72,6 +72,7 @@ public:
 
     bool VisitCompoundStmt(CompoundStmt const*);
     bool VisitCXXOperatorCallExpr(CXXOperatorCallExpr const*);
+    bool VisitCXXMemberCallExpr(CXXMemberCallExpr const*);
 
 private:
     enum class Summands
@@ -262,6 +263,63 @@ bool StringAdd::VisitCXXOperatorCallExpr(CXXOperatorCallExpr const* operatorCall
     return true;
 }
 
+bool StringAdd::VisitCXXMemberCallExpr(CXXMemberCallExpr const* methodCall)
+{
+    if (ignoreLocation(methodCall))
+        return true;
+
+    auto methodDecl = methodCall->getMethodDecl();
+    if (!methodDecl || !methodDecl->getIdentifier() || methodDecl->getName() != "append"
+        || methodCall->getNumArgs() == 0)
+        return true;
+    auto tc1 = loplugin::TypeCheck(methodCall->getType());
+    if (!tc1.Class("OUStringBuffer").Namespace("rtl").GlobalNamespace()
+        && !tc1.Class("OStringBuffer").Namespace("rtl").GlobalNamespace())
+        return true;
+    auto paramType = methodDecl->getParamDecl(0)->getType();
+    // if we convert one of the number append methods, we need to create an extra temporary to hold the string convertion of the number
+    if (paramType->isIntegerType())
+        return true;
+    if (paramType->isCharType())
+        return true;
+    if (paramType->isFloatingType())
+        return true;
+    auto arg = methodCall->getArg(0);
+    // I don't think the OUStringAppend functionality can handle this efficiently
+    if (isa<ConditionalOperator>(ignore(arg)))
+        return true;
+
+    auto methodCall2 = dyn_cast<CXXMemberCallExpr>(ignore(methodCall->getImplicitObjectArgument()));
+    if (!methodCall2)
+        return true;
+    auto tc = loplugin::TypeCheck(methodCall2->getType());
+    if (!tc.Class("OUStringBuffer").Namespace("rtl").GlobalNamespace()
+        && !tc.Class("OStringBuffer").Namespace("rtl").GlobalNamespace())
+        return true;
+    auto methodDecl2 = methodCall2->getMethodDecl();
+    if (!methodDecl2->getIdentifier() || methodDecl2->getName() != "append"
+        || methodCall2->getNumArgs() == 0)
+        return true;
+    auto paramType2 = methodDecl2->getParamDecl(0)->getType();
+    // if we convert one of the number append methods, we need to create an extra temporary to hold the string convertion of the number
+    if (paramType2->isIntegerType())
+        return true;
+    if (paramType2->isCharType())
+        return true;
+    if (paramType2->isFloatingType())
+        return true;
+    arg = methodCall2->getArg(0);
+    // I don't think the OUStringAppend functionality can handle this efficiently
+    if (isa<ConditionalOperator>(ignore(arg)))
+        return true;
+    report(DiagnosticsEngine::Warning,
+           "chained append, rather use single append call and + operator",
+           compat::getBeginLoc(methodCall2))
+        << methodCall2->getSourceRange();
+
+    return true;
+}
+
 Expr const* StringAdd::ignore(Expr const* expr)
 {
     return compat::IgnoreImplicit(compat::IgnoreImplicit(expr)->IgnoreParens());
diff --git a/compilerplugins/clang/stringview.cxx b/compilerplugins/clang/stringview.cxx
index 5df91dcad054..95d7d6368572 100644
--- a/compilerplugins/clang/stringview.cxx
+++ b/compilerplugins/clang/stringview.cxx
@@ -70,7 +70,7 @@ bool StringView::VisitCXXOperatorCallExpr(CXXOperatorCallExpr const* cxxOperator
         if (!memberCallExpr)
             return;
         auto methodDecl = memberCallExpr->getMethodDecl();
-        if (!methodDecl->getIdentifier() || methodDecl->getName() != "copy")
+        if (!methodDecl || !methodDecl->getIdentifier() || methodDecl->getName() != "copy")
             return;
         report(DiagnosticsEngine::Warning, "rather than copy, pass with a view using subView()",
                compat::getBeginLoc(expr))
diff --git a/compilerplugins/clang/test/stringadd.cxx b/compilerplugins/clang/test/stringadd.cxx
index a953e44062bb..a18b562a052f 100644
--- a/compilerplugins/clang/test/stringadd.cxx
+++ b/compilerplugins/clang/test/stringadd.cxx
@@ -217,6 +217,10 @@ void f1(OUString s, OUString t, int i, const char* pChar)
     // no warning expected
     OUString c;
     c = c + OUString(pChar, strlen(pChar), RTL_TEXTENCODING_UTF8);
+
+    OUStringBuffer buf;
+    // expected-error@+1 {{chained append, rather use single append call and + operator [loplugin:stringadd]}}
+    buf.append(" ").append(b);
 }
 void f2(char ch)
 {