[Libreoffice-commits] core.git: compilerplugins/clang solenv/CompilerTest_compilerplugins_clang.mk vbahelper/source
Noel Grandin (via logerrit)
logerrit at kemper.freedesktop.org
Wed Oct 2 09:23:33 UTC 2019
compilerplugins/clang/stringadd.cxx | 273 +++++++++++++++++++++
compilerplugins/clang/test/stringadd.cxx | 150 +++++++++++
solenv/CompilerTest_compilerplugins_clang.mk | 1
vbahelper/source/vbahelper/vbacommandbarhelper.cxx | 4
4 files changed, 425 insertions(+), 3 deletions(-)
New commits:
commit b3b8288f7f6c3cbb36edac2eddf0fff974c6d04a
Author: Noel Grandin <noel.grandin at collabora.co.uk>
AuthorDate: Fri Sep 27 21:41:42 2019 +0200
Commit: Noel Grandin <noel.grandin at collabora.co.uk>
CommitDate: Wed Oct 2 11:22:27 2019 +0200
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 at collabora.co.uk>
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 at +1 {{simplify by merging with the preceding assignment [loplugin:stringadd]}}
+ s2 += "xxx";
+ // expected-error at +1 {{simplify by merging with the preceding assignment [loplugin:stringadd]}}
+ s2 += "xxx";
+ // expected-error at +1 {{simplify by merging with the preceding assignment [loplugin:stringadd]}}
+ s2 += s1;
+ s2 = s1 + "xxx";
+ // expected-error at +1 {{simplify by merging with the preceding assignment [loplugin:stringadd]}}
+ s2 += s1;
+ // expected-error at +1 {{simplify by merging with the preceding assignment [loplugin:stringadd]}}
+ s2 += OUString::number(i);
+ // expected-error at +1 {{simplify by merging with the preceding assignment [loplugin:stringadd]}}
+ s2 += XXX1;
+ // expected-error at +1 {{simplify by merging with the preceding assignment [loplugin:stringadd]}}
+ s2 += OUStringLiteral(XXX1) + XXX2;
+
+ // expected-error at +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 at +1 {{simplify by merging with the preceding assignment [loplugin:stringadd]}}
+ s2 += "xxx";
+ // expected-error at +1 {{simplify by merging with the preceding assignment [loplugin:stringadd]}}
+ s2 += "xxx";
+ // expected-error at +1 {{simplify by merging with the preceding assignment [loplugin:stringadd]}}
+ s2 += s1;
+ s2 = s1 + "xxx";
+ // expected-error at +1 {{simplify by merging with the preceding assignment [loplugin:stringadd]}}
+ s2 += s1;
+ // expected-error at +1 {{simplify by merging with the preceding assignment [loplugin:stringadd]}}
+ s2 += OString::number(i);
+
+ // expected-error at +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 at +1 {{simplify by merging with the preceding assignment [loplugin:stringadd]}}
+ s3 += "xxx";
+}
+void g(OString s3)
+{
+ s3 += "xxx";
+ // expected-error at +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 at +1 {{simplify by merging with the preceding assignment [loplugin:stringadd]}}
+ s3 += b1.m_field;
+ // expected-error at +1 {{simplify by merging with the preceding assignment [loplugin:stringadd]}}
+ s3 += b2.m_field;
+ // expected-error at +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 at +1 {{simplify by merging with the preceding assignment [loplugin:stringadd]}}
+ sValue += sComma + sComma + maExtension + sComma;
+ // expected-error at +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 at +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;
More information about the Libreoffice-commits
mailing list