diff options
| -rw-r--r-- | compilerplugins/clang/stringadd.cxx | 273 | ||||
| -rw-r--r-- | compilerplugins/clang/test/stringadd.cxx | 150 | ||||
| -rw-r--r-- | solenv/CompilerTest_compilerplugins_clang.mk | 1 | ||||
| -rw-r--r-- | vbahelper/source/vbahelper/vbacommandbarhelper.cxx | 4 |
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; |