summaryrefslogtreecommitdiff
path: root/compilerplugins/clang
diff options
context:
space:
mode:
authorNoel Grandin <noel.grandin@collabora.co.uk>2019-10-28 10:35:49 +0200
committerNoel Grandin <noel.grandin@collabora.co.uk>2019-10-28 12:07:54 +0100
commitbc539bd3c04964471af6b5ca54264215c8995696 (patch)
treec37af5b9fb3dc0b8677fc8dd535fb8e93ad6039d /compilerplugins/clang
parent9f78a4174e5099ad3af65a23e158a51c1afca54d (diff)
loplugin:stringadd improve detection
if one side of the expression is a compile-time-constant, we don't need to worry about side-effects on the other side Change-Id: Iee71ea51b327ef244bf39f128f921ac325d74e2b Reviewed-on: https://gerrit.libreoffice.org/81589 Tested-by: Jenkins Reviewed-by: Noel Grandin <noel.grandin@collabora.co.uk>
Diffstat (limited to 'compilerplugins/clang')
-rw-r--r--compilerplugins/clang/stringadd.cxx59
-rw-r--r--compilerplugins/clang/test/stringadd.cxx16
2 files changed, 48 insertions, 27 deletions
diff --git a/compilerplugins/clang/stringadd.cxx b/compilerplugins/clang/stringadd.cxx
index 8b96de07e109..c1023c641bbb 100644
--- a/compilerplugins/clang/stringadd.cxx
+++ b/compilerplugins/clang/stringadd.cxx
@@ -74,11 +74,20 @@ public:
bool VisitCXXOperatorCallExpr(CXXOperatorCallExpr const*);
private:
- const VarDecl* findAssignOrAdd(Stmt const*);
- void checkForCompoundAssign(Stmt const* stmt1, Stmt const* stmt2, VarDecl const* varDecl);
+ struct VarDeclAndConstant
+ {
+ const VarDecl* varDecl;
+ bool bCompileTimeConstant;
+ bool bSideEffectFree;
+ };
+
+ VarDeclAndConstant findAssignOrAdd(Stmt const*);
+ void checkForCompoundAssign(Stmt const* stmt1, Stmt const* stmt2,
+ VarDeclAndConstant const& varDecl);
Expr const* ignore(Expr const*);
bool isSideEffectFree(Expr const*);
+ bool isCompileTimeConstant(Expr const*);
};
bool StringAdd::VisitCompoundStmt(CompoundStmt const* compoundStmt)
@@ -91,9 +100,9 @@ bool StringAdd::VisitCompoundStmt(CompoundStmt const* compoundStmt)
{
if (it == compoundStmt->body_end())
break;
- const VarDecl* foundVar = findAssignOrAdd(*it);
+ VarDeclAndConstant foundVar = findAssignOrAdd(*it);
// reference types have slightly weird behaviour
- if (foundVar && !foundVar->getType()->isReferenceType())
+ if (foundVar.varDecl && !foundVar.varDecl->getType()->isReferenceType())
{
auto stmt1 = *it;
++it;
@@ -109,7 +118,7 @@ bool StringAdd::VisitCompoundStmt(CompoundStmt const* compoundStmt)
return true;
}
-const VarDecl* StringAdd::findAssignOrAdd(Stmt const* stmt)
+StringAdd::VarDeclAndConstant StringAdd::findAssignOrAdd(Stmt const* stmt)
{
if (auto exprCleanup = dyn_cast<ExprWithCleanups>(stmt))
stmt = exprCleanup->getSubExpr();
@@ -123,14 +132,13 @@ const VarDecl* StringAdd::findAssignOrAdd(Stmt const* stmt)
auto tc = loplugin::TypeCheck(varDeclLHS->getType());
if (!tc.Class("OUString").Namespace("rtl").GlobalNamespace()
&& !tc.Class("OString").Namespace("rtl").GlobalNamespace())
- return nullptr;
+ return {};
if (varDeclLHS->getStorageDuration() == SD_Static)
- return nullptr;
+ return {};
if (!varDeclLHS->hasInit())
- return nullptr;
- if (!isSideEffectFree(varDeclLHS->getInit()))
- return nullptr;
- return varDeclLHS;
+ return {};
+ return { varDeclLHS, isCompileTimeConstant(varDeclLHS->getInit()),
+ isSideEffectFree(varDeclLHS->getInit()) };
}
if (auto operatorCall = dyn_cast<CXXOperatorCallExpr>(stmt))
if (operatorCall->getOperator() == OO_Equal || operatorCall->getOperator() == OO_PlusEqual)
@@ -140,16 +148,15 @@ const VarDecl* StringAdd::findAssignOrAdd(Stmt const* stmt)
auto tc = loplugin::TypeCheck(varDeclLHS->getType());
if (!tc.Class("OUString").Namespace("rtl").GlobalNamespace()
&& !tc.Class("OString").Namespace("rtl").GlobalNamespace())
- return nullptr;
+ return {};
auto rhs = operatorCall->getArg(1);
- if (!isSideEffectFree(rhs))
- return nullptr;
- return varDeclLHS;
+ return { varDeclLHS, isCompileTimeConstant(rhs), isSideEffectFree(rhs) };
}
- return nullptr;
+ return {};
}
-void StringAdd::checkForCompoundAssign(Stmt const* stmt1, Stmt const* stmt2, VarDecl const* varDecl)
+void StringAdd::checkForCompoundAssign(Stmt const* stmt1, Stmt const* stmt2,
+ VarDeclAndConstant const& varDecl)
{
// OString additions are frequently wrapped in these
if (auto exprCleanup = dyn_cast<ExprWithCleanups>(stmt2))
@@ -164,10 +171,14 @@ void StringAdd::checkForCompoundAssign(Stmt const* stmt1, Stmt const* stmt2, Var
auto declRefExprLHS = dyn_cast<DeclRefExpr>(ignore(operatorCall->getArg(0)));
if (!declRefExprLHS)
return;
- if (declRefExprLHS->getDecl() != varDecl)
+ if (declRefExprLHS->getDecl() != varDecl.varDecl)
return;
+ // if either side is a compile-time-constant, then we don't care about
+ // side-effects
auto rhs = operatorCall->getArg(1);
- if (!isSideEffectFree(rhs))
+ if (varDecl.bCompileTimeConstant || isCompileTimeConstant(rhs))
+ ; // good
+ else if (!varDecl.bSideEffectFree || !isSideEffectFree(rhs))
return;
// if we cross a #ifdef boundary
if (containsPreprocessingConditionalInclusion(
@@ -304,6 +315,16 @@ bool StringAdd::isSideEffectFree(Expr const* expr)
return false;
}
+bool StringAdd::isCompileTimeConstant(Expr const* expr)
+{
+ expr = compat::IgnoreImplicit(expr);
+ if (auto cxxConstructExpr = dyn_cast<CXXConstructExpr>(expr))
+ if (cxxConstructExpr->getNumArgs() > 0
+ && isa<clang::StringLiteral>(cxxConstructExpr->getArg(0)))
+ return true;
+ return false;
+}
+
loplugin::Plugin::Registration<StringAdd> stringadd("stringadd");
}
diff --git a/compilerplugins/clang/test/stringadd.cxx b/compilerplugins/clang/test/stringadd.cxx
index a8469026e509..f36bbbca999e 100644
--- a/compilerplugins/clang/test/stringadd.cxx
+++ b/compilerplugins/clang/test/stringadd.cxx
@@ -99,6 +99,13 @@ void f(Bar b1, Bar& b2, Bar* b3)
// expected-error@+1 {{simplify by merging with the preceding assignment [loplugin:stringadd]}}
s3 += b3->m_field;
}
+OUString side_effect();
+void f2()
+{
+ OUString sRet = "xxx";
+ // expected-error@+1 {{simplify by merging with the preceding assignment [loplugin:stringadd]}}
+ sRet += side_effect();
+}
}
// no warning expected
@@ -117,17 +124,10 @@ void f()
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";
+ sRet += side_effect();
}
}