diff options
Diffstat (limited to 'compilerplugins/clang/stringadd.cxx')
-rw-r--r-- | compilerplugins/clang/stringadd.cxx | 124 |
1 files changed, 94 insertions, 30 deletions
diff --git a/compilerplugins/clang/stringadd.cxx b/compilerplugins/clang/stringadd.cxx index f10072497767..0ac4ee6d3c65 100644 --- a/compilerplugins/clang/stringadd.cxx +++ b/compilerplugins/clang/stringadd.cxx @@ -16,11 +16,13 @@ #include "plugin.hxx" #include "check.hxx" +#include "compat.hxx" +#include "config_clang.h" #include "clang/AST/CXXInheritance.h" #include "clang/AST/StmtVisitor.h" /** - Look for repeated addition to OUString/OString. + Look for repeated addition to OUString/OString/OUStringBuffer/OStringBuffer. Eg. OUString x = "xxx"; @@ -60,6 +62,9 @@ public: // TODO the += depends on the result of the preceding assign, so can't merge if (fn == SRCDIR "/editeng/source/misc/svxacorr.cxx") return false; + // TODO this file has a boatload of buffer appends' and I don't feel like fixing them all now + if (fn == SRCDIR "/vcl/source/gdi/pdfwriter_impl.cxx") + return false; return true; } @@ -142,12 +147,32 @@ StringAdd::VarDeclAndSummands StringAdd::findAssignOrAdd(Stmt const* stmt) { auto tc = loplugin::TypeCheck(varDeclLHS->getType()); if (!tc.Class("OUString").Namespace("rtl").GlobalNamespace() - && !tc.Class("OString").Namespace("rtl").GlobalNamespace()) + && !tc.Class("OString").Namespace("rtl").GlobalNamespace() + && !tc.Class("OUStringBuffer").Namespace("rtl").GlobalNamespace() + && !tc.Class("OStringBuffer").Namespace("rtl").GlobalNamespace()) return {}; if (varDeclLHS->getStorageDuration() == SD_Static) return {}; if (!varDeclLHS->hasInit()) return {}; + if (tc.Class("OUStringBuffer").Namespace("rtl").GlobalNamespace() + || tc.Class("OStringBuffer").Namespace("rtl").GlobalNamespace()) + { + // ignore the constructor that gives the buffer a default size + if (auto cxxConstructor = dyn_cast<CXXConstructExpr>(varDeclLHS->getInit())) + if (auto constructorDecl = cxxConstructor->getConstructor()) + if ((constructorDecl->getNumParams() == 1 + && loplugin::TypeCheck(constructorDecl->getParamDecl(0)->getType()) + .Typedef("sal_Int32") + .GlobalNamespace()) + || (constructorDecl->getNumParams() == 2 + && constructorDecl->getParamDecl(0)->getType()->isIntegralType( + compiler.getASTContext()) + && constructorDecl->getParamDecl(1) + ->getType() + ->isSpecificBuiltinType(BuiltinType::Int))) + return {}; + } return { varDeclLHS, (isCompileTimeConstant(varDeclLHS->getInit()) ? Summands::OnlyCompileTimeConstants : (isSideEffectFree(varDeclLHS->getInit()) @@ -170,6 +195,24 @@ StringAdd::VarDeclAndSummands StringAdd::findAssignOrAdd(Stmt const* stmt) : (isSideEffectFree(rhs) ? Summands::OnlySideEffectFree : Summands::SideEffect)) }; } + if (auto memberCall = dyn_cast<CXXMemberCallExpr>(stmt)) + if (auto cxxMethodDecl = dyn_cast_or_null<CXXMethodDecl>(memberCall->getDirectCallee())) + if (cxxMethodDecl->getIdentifier() && cxxMethodDecl->getName() == "append") + if (auto declRefExprLHS + = dyn_cast<DeclRefExpr>(ignore(memberCall->getImplicitObjectArgument()))) + if (auto varDeclLHS = dyn_cast<VarDecl>(declRefExprLHS->getDecl())) + { + auto tc = loplugin::TypeCheck(varDeclLHS->getType()); + if (!tc.Class("OUStringBuffer").Namespace("rtl").GlobalNamespace() + && !tc.Class("OStringBuffer").Namespace("rtl").GlobalNamespace()) + return {}; + auto rhs = memberCall->getArg(0); + return { varDeclLHS, + (isCompileTimeConstant(rhs) + ? Summands::OnlyCompileTimeConstants + : (isSideEffectFree(rhs) ? Summands::OnlySideEffectFree + : Summands::SideEffect)) }; + } return {}; } @@ -181,20 +224,41 @@ bool StringAdd::checkForCompoundAssign(Stmt const* stmt1, Stmt const* stmt2, stmt2 = exprCleanup->getSubExpr(); if (auto switchCase = dyn_cast<SwitchCase>(stmt2)) stmt2 = switchCase->getSubStmt(); - auto operatorCall = dyn_cast<CXXOperatorCallExpr>(stmt2); - if (!operatorCall) - return false; - if (operatorCall->getOperator() != OO_PlusEqual) - return false; - auto declRefExprLHS = dyn_cast<DeclRefExpr>(ignore(operatorCall->getArg(0))); + + const DeclRefExpr* declRefExprLHS; + const Expr* rhs; + auto tc = loplugin::TypeCheck(varDecl.varDecl->getType()); + if (tc.Class("OString") || tc.Class("OUString")) + { + auto operatorCall = dyn_cast<CXXOperatorCallExpr>(stmt2); + if (!operatorCall) + return false; + if (operatorCall->getOperator() != OO_PlusEqual) + return false; + declRefExprLHS = dyn_cast<DeclRefExpr>(ignore(operatorCall->getArg(0))); + rhs = operatorCall->getArg(1); + } + else + { + // OUStringBuffer, OStringBuffer + auto memberCall = dyn_cast<CXXMemberCallExpr>(stmt2); + if (!memberCall) + return false; + auto cxxMethodDecl = dyn_cast_or_null<CXXMethodDecl>(memberCall->getDirectCallee()); + if (!cxxMethodDecl) + return false; + if (!cxxMethodDecl->getIdentifier() || cxxMethodDecl->getName() != "append") + return false; + declRefExprLHS = dyn_cast<DeclRefExpr>(ignore(memberCall->getImplicitObjectArgument())); + rhs = memberCall->getArg(0); + } if (!declRefExprLHS) return false; if (declRefExprLHS->getDecl() != varDecl.varDecl) return false; // if either side is a compile-time-constant, then we don't care about // side-effects - auto rhs = operatorCall->getArg(1); - auto const ctcRhs = isCompileTimeConstant(rhs); + bool const ctcRhs = isCompileTimeConstant(rhs); if (!ctcRhs) { auto const sefRhs = isSideEffectFree(rhs); @@ -206,17 +270,24 @@ bool StringAdd::checkForCompoundAssign(Stmt const* stmt1, Stmt const* stmt2, return true; } } + SourceRange mergeRange(stmt1->getSourceRange().getBegin(), stmt2->getSourceRange().getEnd()); // if we cross a #ifdef boundary - if (containsPreprocessingConditionalInclusion( - SourceRange(stmt1->getSourceRange().getBegin(), stmt2->getSourceRange().getEnd()))) + if (containsPreprocessingConditionalInclusion(mergeRange)) { varDecl.summands = ctcRhs ? Summands::OnlyCompileTimeConstants : isSideEffectFree(rhs) ? Summands::OnlySideEffectFree : Summands::SideEffect; return true; } - report(DiagnosticsEngine::Warning, "simplify by merging with the preceding assignment", - compat::getBeginLoc(stmt2)) + // If there is a comment between two calls, rather don't suggest merge + // IMO, code clarity trumps efficiency (as far as plugin warnings go, anyway). + if (containsComment(mergeRange)) + return true; + // I don't think the OUStringAppend functionality can handle this efficiently + if (isa<ConditionalOperator>(ignore(rhs))) + return false; + report(DiagnosticsEngine::Warning, "simplify by merging with the preceding assign/append", + stmt2->getBeginLoc()) << stmt2->getSourceRange(); return true; } @@ -230,8 +301,7 @@ bool StringAdd::VisitCXXOperatorCallExpr(CXXOperatorCallExpr const* operatorCall if (operatorCall->getOperator() != OO_Plus) return true; auto tc = loplugin::TypeCheck(operatorCall->getType()->getUnqualifiedDesugaredType()); - if (!tc.Struct("OUStringConcat").Namespace("rtl").GlobalNamespace() - && !tc.Struct("OStringConcat").Namespace("rtl").GlobalNamespace() + if (!tc.Struct("StringConcat").Namespace("rtl").GlobalNamespace() && !tc.Class("OUString").Namespace("rtl").GlobalNamespace() && !tc.Class("OString").Namespace("rtl").GlobalNamespace()) return true; @@ -253,7 +323,7 @@ bool StringAdd::VisitCXXOperatorCallExpr(CXXOperatorCallExpr const* operatorCall ("rather use O[U]String::Concat than constructing %0 from %1 on %select{L|R}2HS of " "+ (where %select{R|L}2HS is of" " type %3)"), - compat::getBeginLoc(e)) + e->getBeginLoc()) << e->getType().getLocalUnqualifiedType() << e->getSubExprAsWritten()->getType() << arg << operatorCall->getArg(1 - arg)->IgnoreImpCasts()->getType() << e->getSourceRange(); }; @@ -276,10 +346,6 @@ bool StringAdd::VisitCXXMemberCallExpr(CXXMemberCallExpr const* methodCall) if (!tc1.Class("OUStringBuffer").Namespace("rtl").GlobalNamespace() && !tc1.Class("OStringBuffer").Namespace("rtl").GlobalNamespace()) return true; - auto paramType = methodDecl->getParamDecl(0)->getType(); - // char is still a pain to work with, when constructing a chained + - if (paramType->isCharType() || loplugin::TypeCheck(paramType).Typedef("sal_Unicode")) - return true; auto arg = methodCall->getArg(0); // I don't think the OUStringAppend functionality can handle this efficiently if (isa<ConditionalOperator>(ignore(arg))) @@ -296,17 +362,13 @@ bool StringAdd::VisitCXXMemberCallExpr(CXXMemberCallExpr const* methodCall) if (!methodDecl2->getIdentifier() || methodDecl2->getName() != "append" || methodCall2->getNumArgs() == 0) return true; - auto paramType2 = methodDecl2->getParamDecl(0)->getType(); - // char is still a pain to work with, when constructing a chained + - if (paramType2->isCharType() || loplugin::TypeCheck(paramType2).Typedef("sal_Unicode")) - 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->getBeginLoc()) << methodCall2->getSourceRange(); return true; @@ -314,7 +376,7 @@ bool StringAdd::VisitCXXMemberCallExpr(CXXMemberCallExpr const* methodCall) Expr const* StringAdd::ignore(Expr const* expr) { - return compat::IgnoreImplicit(compat::IgnoreImplicit(expr)->IgnoreParens()); + return expr->IgnoreImplicit()->IgnoreParens()->IgnoreImplicit(); } bool StringAdd::isSideEffectFree(Expr const* expr) @@ -417,7 +479,7 @@ bool StringAdd::isSideEffectFree(Expr const* expr) if (isSideEffectFree(callExpr->getArg(0))) return true; // allowlist some known-safe methods - if (name.endswith("ResId") || name == "GetXMLToken") + if (compat::ends_with(name, "ResId") || name == "GetXMLToken") if (isSideEffectFree(callExpr->getArg(0))) return true; } @@ -427,7 +489,9 @@ bool StringAdd::isSideEffectFree(Expr const* expr) if (auto constructExpr = dyn_cast<CXXConstructExpr>(expr)) { auto dc = loplugin::DeclCheck(constructExpr->getConstructor()); - if (dc.MemberFunction().Class("OUString") || dc.MemberFunction().Class("OString")) + if (dc.MemberFunction().Class("OUString") || dc.MemberFunction().Class("OString") + || dc.MemberFunction().Class("OUStringBuffer") + || dc.MemberFunction().Class("OStringBuffer")) if (constructExpr->getNumArgs() == 0 || isSideEffectFree(constructExpr->getArg(0))) return true; // Expr::HasSideEffects does not like stuff that passes through OUStringLiteral @@ -451,7 +515,7 @@ bool StringAdd::isSideEffectFree(Expr const* expr) bool StringAdd::isCompileTimeConstant(Expr const* expr) { - expr = compat::IgnoreImplicit(expr); + expr = expr->IgnoreImplicit(); if (auto cxxConstructExpr = dyn_cast<CXXConstructExpr>(expr)) if (cxxConstructExpr->getNumArgs() > 0) expr = cxxConstructExpr->getArg(0); |