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>
This commit is contained in:
Noel Grandin
273
compilerplugins/clang/stringadd.cxx
Normal file
273
compilerplugins/clang/stringadd.cxx
Normal file
@@ -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: */
|
||||
150
compilerplugins/clang/test/stringadd.cxx
Normal file
150
compilerplugins/clang/test/stringadd.cxx
Normal file
@@ -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: */
|
||||
Reference in New Issue
Block a user