mir/libreoffice
2
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

new loplugin:bufferadd

Browse Source 
look for OUStringBuffer append sequences that can be turned
into creating an OUString with + operations

Change-Id: Ica840dc096000307b4a105fb4d9ec7588a15ade6
Reviewed-on: https://gerrit.libreoffice.org/80809
Tested-by: Jenkins
Reviewed-by: Noel Grandin <noel.grandin@collabora.co.uk>
This commit is contained in:
Noel Grandin
2019-10-14 14:27:57 +02:00
parent 9270f74466
commit f13c6ad5f0
126 changed files with 1022 additions and 856 deletions
Download Patch File Download Diff File Expand all files Collapse all files

416
compilerplugins/clang/bufferadd.cxx Normal file
View File

@@ -0,0 +1,416 @@
/* -*- 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_set>
#include "plugin.hxx"
#include "check.hxx"
#include "clang/AST/CXXInheritance.h"
#include "clang/AST/StmtVisitor.h"
/**
Look for *StringBuffer append sequences which can be converted to *String + sequences.
*/
namespace
{
class BufferAdd : public loplugin::FilteringPlugin<BufferAdd>
{
public:
explicit BufferAdd(loplugin::InstantiationData const& data)
: FilteringPlugin(data)
{
}
bool preRun() override
{
std::string fn(handler.getMainFileName());
loplugin::normalizeDotDotInFilePath(fn);
if (loplugin::hasPathnamePrefix(fn, SRCDIR "/sal/qa/rtl/oustring/"))
return false;
if (loplugin::hasPathnamePrefix(fn, SRCDIR "/sal/qa/rtl/oustringbuffer/"))
return false;
if (loplugin::hasPathnamePrefix(fn, SRCDIR "/sal/qa/rtl/strings/"))
return false;
if (loplugin::hasPathnamePrefix(fn, SRCDIR "/sal/qa/OStringBuffer/"))
return false;
// some false +
if (loplugin::isSamePathname(fn, SRCDIR "/unoidl/source/sourcetreeprovider.cxx"))
return false;
if (loplugin::isSamePathname(fn, SRCDIR "/writerfilter/source/dmapper/StyleSheetTable.cxx"))
return false;
if (loplugin::isSamePathname(fn, SRCDIR "/writerfilter/source/dmapper/GraphicImport.cxx"))
return false;
return true;
}
virtual void run() override
{
if (!preRun())
return;
TraverseDecl(compiler.getASTContext().getTranslationUnitDecl());
for (auto const& pair : goodMap)
if (!isa<ParmVarDecl>(pair.first) &&
// reference types have slightly weird behaviour
!pair.first->getType()->isReferenceType()
&& badMap.find(pair.first) == badMap.end())
report(DiagnosticsEngine::Warning,
"convert this append sequence into a *String + sequence",
compat::getBeginLoc(pair.first))
<< pair.first->getSourceRange();
}
bool VisitStmt(Stmt const*);
bool VisitCallExpr(CallExpr const*);
bool VisitCXXConstructExpr(CXXConstructExpr const*);
bool VisitUnaryOperator(UnaryOperator const*);
private:
void findBufferAssignOrAdd(const Stmt* parentStmt, Stmt const*);
Expr const* ignore(Expr const*);
bool isSideEffectFree(Expr const*);
bool isMethodOkToMerge(CXXMemberCallExpr const*);
void addToGoodMap(const VarDecl* varDecl, const Stmt* parentStmt);
std::unordered_map<const VarDecl*, const Stmt*> goodMap;
std::unordered_set<const VarDecl*> badMap;
};
bool BufferAdd::VisitStmt(Stmt const* stmt)
{
if (ignoreLocation(stmt))
return true;
if (!isa<CompoundStmt>(stmt) && !isa<CXXCatchStmt>(stmt) && !isa<CXXForRangeStmt>(stmt)
&& !isa<CXXTryStmt>(stmt) && !isa<DoStmt>(stmt) && !isa<ForStmt>(stmt) && !isa<IfStmt>(stmt)
&& !isa<SwitchStmt>(stmt) && !isa<WhileStmt>(stmt))
return true;
for (auto it = stmt->child_begin(); it != stmt->child_end(); ++it)
if (*it)
findBufferAssignOrAdd(stmt, *it);
return true;
}
bool BufferAdd::VisitCallExpr(CallExpr const* callExpr)
{
if (ignoreLocation(callExpr))
return true;
for (unsigned i = 0; i != callExpr->getNumArgs(); ++i)
{
auto a = ignore(callExpr->getArg(i));
if (auto declRefExpr = dyn_cast<DeclRefExpr>(a))
if (auto varDecl = dyn_cast<VarDecl>(declRefExpr->getDecl()))
badMap.insert(varDecl);
}
return true;
}
bool BufferAdd::VisitCXXConstructExpr(CXXConstructExpr const* callExpr)
{
if (ignoreLocation(callExpr))
return true;
for (unsigned i = 0; i != callExpr->getNumArgs(); ++i)
{
auto a = ignore(callExpr->getArg(i));
if (auto declRefExpr = dyn_cast<DeclRefExpr>(a))
if (auto varDecl = dyn_cast<VarDecl>(declRefExpr->getDecl()))
badMap.insert(varDecl);
}
return true;
}
bool BufferAdd::VisitUnaryOperator(const UnaryOperator* unaryOp)
{
if (ignoreLocation(unaryOp))
return true;
if (unaryOp->getOpcode() != UO_AddrOf)
return true;
auto a = ignore(unaryOp->getSubExpr());
if (auto declRefExpr = dyn_cast<DeclRefExpr>(a))
if (auto varDecl = dyn_cast<VarDecl>(declRefExpr->getDecl()))
badMap.insert(varDecl);
return true;
}
void BufferAdd::findBufferAssignOrAdd(const Stmt* parentStmt, 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("OUStringBuffer").Namespace("rtl").GlobalNamespace()
&& !tc.Class("OStringBuffer").Namespace("rtl").GlobalNamespace())
return;
if (varDeclLHS->getStorageDuration() == SD_Static)
return;
if (!varDeclLHS->hasInit())
return;
auto cxxConstructExpr = dyn_cast<CXXConstructExpr>(ignore(varDeclLHS->getInit()));
if (cxxConstructExpr)
{
if (cxxConstructExpr->getNumArgs() == 0)
{
addToGoodMap(varDeclLHS, parentStmt);
return;
}
auto tc2 = loplugin::TypeCheck(cxxConstructExpr->getArg(0)->getType());
if (cxxConstructExpr->getArg(0)->getType()->isBuiltinType()
|| tc2.LvalueReference().Class("OUStringBuffer")
|| tc2.LvalueReference().Class("OStringBuffer")
|| tc2.Class("OUStringBuffer") || tc2.Class("OStringBuffer"))
{
badMap.insert(varDeclLHS);
return;
}
}
if (!isSideEffectFree(varDeclLHS->getInit()))
badMap.insert(varDeclLHS);
else
addToGoodMap(varDeclLHS, parentStmt);
}
return;
}
// check for single calls to buffer method
if (auto memberCallExpr = dyn_cast<CXXMemberCallExpr>(stmt))
{
if (auto declRefExprLHS
= dyn_cast<DeclRefExpr>(ignore(memberCallExpr->getImplicitObjectArgument())))
{
auto methodDecl = memberCallExpr->getMethodDecl();
if (methodDecl && methodDecl->getIdentifier())
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())
{
if (isMethodOkToMerge(memberCallExpr))
addToGoodMap(varDeclLHS, parentStmt);
else
badMap.insert(varDeclLHS);
}
}
return;
}
}
// now check for chained append calls
auto expr = dyn_cast<Expr>(stmt);
if (!expr)
return;
auto tc = loplugin::TypeCheck(expr->getType());
if (!tc.Class("OUStringBuffer").Namespace("rtl").GlobalNamespace()
&& !tc.Class("OStringBuffer").Namespace("rtl").GlobalNamespace())
return;
// unwrap the chain (which runs from right to left)
const VarDecl* varDeclLHS = nullptr;
bool good = true;
while (true)
{
auto memberCallExpr = dyn_cast<CXXMemberCallExpr>(expr);
if (!memberCallExpr)
break;
good &= isMethodOkToMerge(memberCallExpr);
if (auto declRefExprLHS
= dyn_cast<DeclRefExpr>(ignore(memberCallExpr->getImplicitObjectArgument())))
{
varDeclLHS = dyn_cast<VarDecl>(declRefExprLHS->getDecl());
break;
}
expr = memberCallExpr->getImplicitObjectArgument();
}
if (varDeclLHS)
{
if (good)
addToGoodMap(varDeclLHS, parentStmt);
else
badMap.insert(varDeclLHS);
}
}
void BufferAdd::addToGoodMap(const VarDecl* varDecl, const Stmt* parentStmt)
{
// check that vars are all inside the same compoundstmt, if they are not, we cannot combine them
auto it = goodMap.find(varDecl);
if (it != goodMap.end())
{
if (it->second == parentStmt)
return;
// don't treat these as parents, otherwise we eliminate .append.append sequences
if (isa<MemberExpr>(parentStmt))
return;
if (isa<CXXMemberCallExpr>(parentStmt))
return;
badMap.insert(varDecl);
}
else
goodMap.emplace(varDecl, parentStmt);
}
bool BufferAdd::isMethodOkToMerge(CXXMemberCallExpr const* memberCall)
{
auto methodDecl = memberCall->getMethodDecl();
if (methodDecl->getNumParams() == 0)
return true;
auto tc2 = loplugin::TypeCheck(methodDecl->getParamDecl(0)->getType());
if (tc2.LvalueReference().Class("OUStringBuffer")
|| tc2.LvalueReference().Class("OStringBuffer"))
return false;
auto name = methodDecl->getName();
if (name == "appendUninitialized" || name == "setLength" || name == "remove" || name == "insert"
|| name == "appendAscii" || name == "appendUtf32")
return false;
auto rhs = memberCall->getArg(0);
if (loplugin::TypeCheck(memberCall->getType())
.Class("OStringBuffer")
.Namespace("rtl")
.GlobalNamespace())
{
// because we have no OStringLiteral1
if (tc2.Char())
return false;
// Can't see how to make the call to append(sal_Unicode*pStart, sal_Unicode*pEnd) work
if (memberCall->getNumArgs() == 2 && loplugin::TypeCheck(rhs->getType()).Pointer())
return false;
}
if (loplugin::TypeCheck(memberCall->getType())
.Class("OUStringBuffer")
.Namespace("rtl")
.GlobalNamespace())
{
// character literals we do with OUStringBuffer, not variables of type sal_Unicode/char
if (tc2.Typedef("sal_Unicode").GlobalNamespace() && !isa<CharacterLiteral>(rhs))
return false;
// Can't see how to make the call to append(sal_Unicode*pStart, sal_Unicode*pEnd) work
if (memberCall->getNumArgs() == 2
&& loplugin::TypeCheck(memberCall->getArg(0)->getType()).Pointer())
return false;
}
if (!isSideEffectFree(rhs))
return false;
return true;
}
Expr const* BufferAdd::ignore(Expr const* expr)
{
return compat::IgnoreImplicit(compat::IgnoreImplicit(expr)->IgnoreParens());
}
bool BufferAdd::isSideEffectFree(Expr const* expr)
{
expr = ignore(expr);
// I don't think the OUStringAppend functionality can handle this efficiently
if (isa<ConditionalOperator>(expr))
return false;
// 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/OUString::unacquired
if (auto calleeMethodDecl = dyn_cast_or_null<CXXMethodDecl>(callExpr->getCalleeDecl()))
if (calleeMethodDecl && calleeMethodDecl->getIdentifier())
{
auto name = calleeMethodDecl->getName();
if (name == "number" || name == "unacquired")
{
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;
// whitelist some known-safe methods
if (name.endswith("ResId") || name == "GetXMLToken")
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")
|| 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
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;
}
loplugin::Plugin::Registration<BufferAdd> bufferadd("bufferadd");
}
#endif // LO_CLANG_SHARED_PLUGINS
/* vim:set shiftwidth=4 softtabstop=4 expandtab: */

75
compilerplugins/clang/test/bufferadd.cxx Normal file
View File

@@ -0,0 +1,75 @@
/* -*- 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/strbuf.hxx>
#include <rtl/string.hxx>
#include <rtl/ustrbuf.hxx>
#include <rtl/ustring.hxx>
// ---------------------------------------------------------------
// replaceing OUStringBuffer.append sequences to OUString+
namespace test1
{
void f1()
{
// expected-error@+1 {{convert this append sequence into a *String + sequence [loplugin:bufferadd]}}
OUStringBuffer v;
v.append("xxx");
v.append("xxx");
}
void f2()
{
// expected-error@+1 {{convert this append sequence into a *String + sequence [loplugin:bufferadd]}}
OUStringBuffer v;
v.append("xxx").append("aaaa");
}
}
namespace test2
{
void f2()
{
// no warning expected
OUStringBuffer v;
v.append("xxx");
if (true)
v.append("yyyy");
}
void appendTo(OUStringBuffer&);
void f3()
{
// no warning expected
OUStringBuffer v;
appendTo(v);
v.append("xxx");
}
void f4()
{
// no warning expected
OUStringBuffer v;
v.append("xxx");
v.setLength(0);
}
void f5()
{
// no warning expected
OUStringBuffer v;
v.append("xxx");
v[1] = 'x';
}
void f6()
{
// no warning expected
OUStringBuffer noel1("xxx");
while (true)
noel1.append("ffff").append("aaa");
}
}
/* vim:set shiftwidth=4 softtabstop=4 expandtab cinoptions=b1,g0,N-s cinkeys+=0=break: */