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

teach useuniqueptr loplugin about while loops

Browse Source 
and reduce code duplication

Change-Id: I292d7515b15fce4cf1714c3b11b947493706bc3c
Reviewed-on: https://gerrit.libreoffice.org/49090
Reviewed-by: Noel Grandin <noel.grandin@collabora.co.uk>
Tested-by: Noel Grandin <noel.grandin@collabora.co.uk>
This commit is contained in:
Noel Grandin
2018-01-17 13:14:21 +02:00
parent 056ee671e2
commit 1d98683296
2 changed files with 126 additions and 135 deletions
Download Patch File Download Diff File Expand all files Collapse all files

16
compilerplugins/clang/test/useuniqueptr.cxx
View File

@@ -99,6 +99,7 @@ class Foo9 {
XXX* m_pbar1; // expected-note {{member is here [loplugin:useuniqueptr]}} XXX* m_pbar1; // expected-note {{member is here [loplugin:useuniqueptr]}}
XXX* m_pbar2; // expected-note {{member is here [loplugin:useuniqueptr]}} XXX* m_pbar2; // expected-note {{member is here [loplugin:useuniqueptr]}}
XXX* m_pbar3; // expected-note {{member is here [loplugin:useuniqueptr]}} XXX* m_pbar3; // expected-note {{member is here [loplugin:useuniqueptr]}}
XXX* m_pbar4; // expected-note {{member is here [loplugin:useuniqueptr]}}
~Foo9() ~Foo9()
{ {
if (m_pbar1) if (m_pbar1)
@@ -111,6 +112,12 @@ class Foo9 {
} }
if (m_pbar3 != nullptr) if (m_pbar3 != nullptr)
delete m_pbar3; // expected-error {{unconditional call to delete on a member, should be using std::unique_ptr [loplugin:useuniqueptr]}} delete m_pbar3; // expected-error {{unconditional call to delete on a member, should be using std::unique_ptr [loplugin:useuniqueptr]}}
if (m_pbar4 != nullptr)
{
int x = 1;
(void)x;
delete m_pbar4; // expected-error {{unconditional call to delete on a member, should be using std::unique_ptr [loplugin:useuniqueptr]}}
}
} }
}; };
// no warning expected // no warning expected
@@ -135,4 +142,13 @@ class Foo11 {
} }
} }
}; };
class Foo12 {
std::array<int*,10> m_pbar; // expected-note {{member is here [loplugin:useuniqueptr]}}
~Foo12()
{
int i = 0;
while (i < 10)
delete m_pbar[i++]; // expected-error {{rather manage with std::some_container<std::unique_ptr<T>> [loplugin:useuniqueptr]}}
}
};
/* vim:set shiftwidth=4 softtabstop=4 expandtab cinoptions=b1,g0,N-s cinkeys+=0=break: */ /* vim:set shiftwidth=4 softtabstop=4 expandtab cinoptions=b1,g0,N-s cinkeys+=0=break: */

183
compilerplugins/clang/useuniqueptr.cxx
View File

@@ -39,9 +39,13 @@ public:
bool VisitCompoundStmt(const CompoundStmt* ); bool VisitCompoundStmt(const CompoundStmt* );
private: private:
void CheckForUnconditionalDelete(const CXXDestructorDecl*, const CompoundStmt* ); void CheckForUnconditionalDelete(const CXXDestructorDecl*, const CompoundStmt* );
void CheckDeleteExpr(const CXXDestructorDecl*, const CXXDeleteExpr* ); void CheckForSimpleDelete(const CXXDestructorDecl*, const CompoundStmt* );
void CheckForForLoopDelete(const CXXDestructorDecl*, const CompoundStmt* ); void CheckRangedLoopDelete(const CXXDestructorDecl*, const CXXForRangeStmt* );
void CheckForRangedLoopDelete(const CXXDestructorDecl*, const CompoundStmt* ); void CheckLoopDelete(const CXXDestructorDecl*, const Stmt* );
void CheckLoopDelete(const CXXDestructorDecl*, const CXXDeleteExpr* );
void CheckDeleteExpr(const CXXDestructorDecl*, const CXXDeleteExpr*);
void CheckDeleteExpr(const CXXDestructorDecl*, const CXXDeleteExpr*,
const MemberExpr*, StringRef message);
}; };
bool UseUniquePtr::VisitCXXDestructorDecl(const CXXDestructorDecl* destructorDecl) bool UseUniquePtr::VisitCXXDestructorDecl(const CXXDestructorDecl* destructorDecl)
@@ -55,14 +59,25 @@ bool UseUniquePtr::VisitCXXDestructorDecl(const CXXDestructorDecl* destructorDec
if (!compoundStmt || compoundStmt->size() == 0) if (!compoundStmt || compoundStmt->size() == 0)
return true; return true;
CheckForUnconditionalDelete(destructorDecl, compoundStmt); CheckForSimpleDelete(destructorDecl, compoundStmt);
CheckForForLoopDelete(destructorDecl, compoundStmt);
CheckForRangedLoopDelete(destructorDecl, compoundStmt); for (auto i = compoundStmt->body_begin(); i != compoundStmt->body_end(); ++i)
{
if (auto cxxForRangeStmt = dyn_cast<CXXForRangeStmt>(*i))
CheckRangedLoopDelete(destructorDecl, cxxForRangeStmt);
else if (auto forStmt = dyn_cast<ForStmt>(*i))
CheckLoopDelete(destructorDecl, forStmt->getBody());
else if (auto whileStmt = dyn_cast<WhileStmt>(*i))
CheckLoopDelete(destructorDecl, whileStmt->getBody());
}
return true; return true;
} }
void UseUniquePtr::CheckForUnconditionalDelete(const CXXDestructorDecl* destructorDecl, const CompoundStmt* compoundStmt) /**
* check for simple call to delete in a destructor i.e. direct unconditional call, or if-guarded call
*/
void UseUniquePtr::CheckForSimpleDelete(const CXXDestructorDecl* destructorDecl, const CompoundStmt* compoundStmt)
{ {
for (auto i = compoundStmt->body_begin(); i != compoundStmt->body_end(); ++i) for (auto i = compoundStmt->body_begin(); i != compoundStmt->body_end(); ++i)
{ {
@@ -83,6 +98,7 @@ void UseUniquePtr::CheckForUnconditionalDelete(const CXXDestructorDecl* destruct
// ignore "if (bMine)" // ignore "if (bMine)"
if (!loplugin::TypeCheck(ifCondMemberExpr->getType()).Pointer()) if (!loplugin::TypeCheck(ifCondMemberExpr->getType()).Pointer())
continue; continue;
// good
} }
else if (auto binaryOp = dyn_cast<BinaryOperator>(cond)) else if (auto binaryOp = dyn_cast<BinaryOperator>(cond))
{ {
@@ -90,15 +106,18 @@ void UseUniquePtr::CheckForUnconditionalDelete(const CXXDestructorDecl* destruct
continue; continue;
if (!isa<CXXNullPtrLiteralExpr>(binaryOp->getRHS()->IgnoreImpCasts())) if (!isa<CXXNullPtrLiteralExpr>(binaryOp->getRHS()->IgnoreImpCasts()))
continue; continue;
// good
} }
else else // ignore anything more complicated
continue; continue;
deleteExpr = dyn_cast<CXXDeleteExpr>(ifStmt->getThen()); deleteExpr = dyn_cast<CXXDeleteExpr>(ifStmt->getThen());
if (deleteExpr) if (deleteExpr)
{ {
CheckDeleteExpr(destructorDecl, deleteExpr); CheckDeleteExpr(destructorDecl, deleteExpr);
continue; continue;
} }
auto ifThenCompoundStmt = dyn_cast<CompoundStmt>(ifStmt->getThen()); auto ifThenCompoundStmt = dyn_cast<CompoundStmt>(ifStmt->getThen());
if (!ifThenCompoundStmt) if (!ifThenCompoundStmt)
continue; continue;
@@ -116,29 +135,38 @@ void UseUniquePtr::CheckForUnconditionalDelete(const CXXDestructorDecl* destruct
*/ */
void UseUniquePtr::CheckDeleteExpr(const CXXDestructorDecl* destructorDecl, const CXXDeleteExpr* deleteExpr) void UseUniquePtr::CheckDeleteExpr(const CXXDestructorDecl* destructorDecl, const CXXDeleteExpr* deleteExpr)
{ {
const ImplicitCastExpr* pCastExpr = dyn_cast<ImplicitCastExpr>(deleteExpr->getArgument()); const ImplicitCastExpr* castExpr = dyn_cast<ImplicitCastExpr>(deleteExpr->getArgument());
if (!pCastExpr) if (!castExpr)
return; return;
const MemberExpr* pMemberExpr = dyn_cast<MemberExpr>(pCastExpr->getSubExpr()); const MemberExpr* memberExpr = dyn_cast<MemberExpr>(castExpr->getSubExpr());
if (!pMemberExpr) if (!memberExpr)
return; return;
CheckDeleteExpr(destructorDecl, deleteExpr, memberExpr,
"unconditional call to delete on a member, should be using std::unique_ptr");
}
/**
* Check the delete expression in a destructor.
*/
void UseUniquePtr::CheckDeleteExpr(const CXXDestructorDecl* destructorDecl, const CXXDeleteExpr* deleteExpr,
const MemberExpr* memberExpr, StringRef message)
{
// ignore union games // ignore union games
const FieldDecl* pFieldDecl = dyn_cast<FieldDecl>(pMemberExpr->getMemberDecl()); const FieldDecl* fieldDecl = dyn_cast<FieldDecl>(memberExpr->getMemberDecl());
if (!pFieldDecl) if (!fieldDecl)
return; return;
TagDecl const * td = dyn_cast<TagDecl>(pFieldDecl->getDeclContext()); TagDecl const * td = dyn_cast<TagDecl>(fieldDecl->getDeclContext());
if (td->isUnion()) if (td->isUnion())
return; return;
// ignore calling delete on someone else's field // ignore calling delete on someone else's field
if (pFieldDecl->getParent() != destructorDecl->getParent() ) if (fieldDecl->getParent() != destructorDecl->getParent() )
return; return;
if (ignoreLocation(pFieldDecl)) if (ignoreLocation(fieldDecl))
return; return;
// to ignore things like the CPPUNIT macros // to ignore things like the CPPUNIT macros
StringRef aFileName = compiler.getSourceManager().getFilename(compiler.getSourceManager().getSpellingLoc(pFieldDecl->getLocStart())); StringRef aFileName = compiler.getSourceManager().getFilename(compiler.getSourceManager().getSpellingLoc(fieldDecl->getLocStart()));
if (loplugin::hasPathnamePrefix(aFileName, WORKDIR)) if (loplugin::hasPathnamePrefix(aFileName, WORKDIR))
return; return;
// passes and stores pointers to member fields // passes and stores pointers to member fields
@@ -169,30 +197,48 @@ void UseUniquePtr::CheckDeleteExpr(const CXXDestructorDecl* destructorDecl, cons
return; return;
if (loplugin::hasPathnamePrefix(aFileName, SRCDIR "/include/sot/")) if (loplugin::hasPathnamePrefix(aFileName, SRCDIR "/include/sot/"))
return; return;
// the std::vector is being passed to another class
if (loplugin::hasPathnamePrefix(aFileName, SRCDIR "/sfx2/source/explorer/nochaos.cxx"))
return;
// ignore std::map and std::unordered_map, MSVC 2015 has problems with mixing these with std::unique_ptr
auto tc = loplugin::TypeCheck(fieldDecl->getType());
if (tc.Class("map").StdNamespace() || tc.Class("unordered_map").StdNamespace())
return;
// there is a loop in ~ImplPrnQueueList deleting stuff on a global data structure
if (loplugin::hasPathnamePrefix(aFileName, SRCDIR "/vcl/inc/print.h"))
return;
// painful linked list
if (loplugin::hasPathnamePrefix(aFileName, SRCDIR "/basic/source/inc/runtime.hxx"))
return;
report( report(
DiagnosticsEngine::Warning, DiagnosticsEngine::Warning,
"unconditional call to delete on a member, should be using std::unique_ptr", message,
deleteExpr->getLocStart()) deleteExpr->getLocStart())
<< deleteExpr->getSourceRange(); << deleteExpr->getSourceRange();
report( report(
DiagnosticsEngine::Note, DiagnosticsEngine::Note,
"member is here", "member is here",
pFieldDecl->getLocStart()) fieldDecl->getLocStart())
<< pFieldDecl->getSourceRange(); << fieldDecl->getSourceRange();
} }
void UseUniquePtr::CheckForForLoopDelete(const CXXDestructorDecl* destructorDecl, const CompoundStmt* compoundStmt) void UseUniquePtr::CheckLoopDelete(const CXXDestructorDecl* destructorDecl, const Stmt* bodyStmt)
{ {
if (auto deleteExpr = dyn_cast<CXXDeleteExpr>(bodyStmt))
CheckLoopDelete(destructorDecl, deleteExpr);
else if (auto compoundStmt = dyn_cast<CompoundStmt>(bodyStmt))
{
for (auto i = compoundStmt->body_begin(); i != compoundStmt->body_end(); ++i) for (auto i = compoundStmt->body_begin(); i != compoundStmt->body_end(); ++i)
{ {
auto forStmt = dyn_cast<ForStmt>(*i); if (auto deleteExpr = dyn_cast<CXXDeleteExpr>(*i))
if (!forStmt) CheckLoopDelete(destructorDecl, deleteExpr);
continue; }
auto deleteExpr = dyn_cast<CXXDeleteExpr>(forStmt->getBody()); }
if (!deleteExpr) }
continue;
void UseUniquePtr::CheckLoopDelete(const CXXDestructorDecl* destructorDecl, const CXXDeleteExpr* deleteExpr)
{
const MemberExpr* memberExpr = nullptr; const MemberExpr* memberExpr = nullptr;
const Expr* subExpr = deleteExpr->getArgument(); const Expr* subExpr = deleteExpr->getArgument();
for (;;) for (;;)
@@ -215,99 +261,28 @@ void UseUniquePtr::CheckForForLoopDelete(const CXXDestructorDecl* destructorDecl
break; break;
} }
if (!memberExpr) if (!memberExpr)
continue;
// ignore union games
const FieldDecl* fieldDecl = dyn_cast<FieldDecl>(memberExpr->getMemberDecl());
if (!fieldDecl)
continue;
TagDecl const * td = dyn_cast<TagDecl>(fieldDecl->getDeclContext());
if (td->isUnion())
continue;
// ignore calling delete on someone else's field
if (fieldDecl->getParent() != destructorDecl->getParent() )
continue;
if (ignoreLocation(fieldDecl))
continue;
// to ignore things like the CPPUNIT macros
StringRef aFileName = compiler.getSourceManager().getFilename(compiler.getSourceManager().getSpellingLoc(fieldDecl->getLocStart()));
if (loplugin::hasPathnamePrefix(aFileName, WORKDIR))
continue;
// the std::vector is being passed to another class
if (loplugin::hasPathnamePrefix(aFileName, SRCDIR "/sfx2/source/explorer/nochaos.cxx"))
return; return;
report( CheckDeleteExpr(destructorDecl, deleteExpr, memberExpr, "rather manage with std::some_container<std::unique_ptr<T>>");
DiagnosticsEngine::Warning,
"rather manage with std::some_container<std::unique_ptr<T>>",
deleteExpr->getLocStart())
<< deleteExpr->getSourceRange();
report(
DiagnosticsEngine::Note,
"member is here",
fieldDecl->getLocStart())
<< fieldDecl->getSourceRange();
}
} }
void UseUniquePtr::CheckForRangedLoopDelete(const CXXDestructorDecl* destructorDecl, const CompoundStmt* compoundStmt) void UseUniquePtr::CheckRangedLoopDelete(const CXXDestructorDecl* destructorDecl, const CXXForRangeStmt* cxxForRangeStmt)
{ {
for (auto i = compoundStmt->body_begin(); i != compoundStmt->body_end(); ++i)
{
auto cxxForRangeStmt = dyn_cast<CXXForRangeStmt>(*i);
if (!cxxForRangeStmt)
continue;
CXXDeleteExpr const * deleteExpr = nullptr; CXXDeleteExpr const * deleteExpr = nullptr;
if (auto compoundStmt = dyn_cast<CompoundStmt>(cxxForRangeStmt->getBody())) if (auto compoundStmt = dyn_cast<CompoundStmt>(cxxForRangeStmt->getBody()))
deleteExpr = dyn_cast<CXXDeleteExpr>(*compoundStmt->body_begin()); deleteExpr = dyn_cast<CXXDeleteExpr>(*compoundStmt->body_begin());
else else
deleteExpr = dyn_cast<CXXDeleteExpr>(cxxForRangeStmt->getBody()); deleteExpr = dyn_cast<CXXDeleteExpr>(cxxForRangeStmt->getBody());
if (!deleteExpr) if (!deleteExpr)
continue; return;
auto memberExpr = dyn_cast<MemberExpr>(cxxForRangeStmt->getRangeInit()); auto memberExpr = dyn_cast<MemberExpr>(cxxForRangeStmt->getRangeInit());
if (!memberExpr) if (!memberExpr)
continue; return;
auto fieldDecl = dyn_cast<FieldDecl>(memberExpr->getMemberDecl()); auto fieldDecl = dyn_cast<FieldDecl>(memberExpr->getMemberDecl());
if (!fieldDecl) if (!fieldDecl)
continue;
// ignore union games
TagDecl const * td = dyn_cast<TagDecl>(fieldDecl->getDeclContext());
if (td->isUnion())
continue;
// ignore calling delete on someone else's field
if (fieldDecl->getParent() != destructorDecl->getParent() )
continue;
if (ignoreLocation(fieldDecl))
continue;
// to ignore things like the CPPUNIT macros
StringRef aFileName = compiler.getSourceManager().getFilename(compiler.getSourceManager().getSpellingLoc(fieldDecl->getLocStart()));
if (loplugin::hasPathnamePrefix(aFileName, WORKDIR))
continue;
// ignore std::map and std::unordered_map, MSVC 2015 has problems with mixing these with std::unique_ptr
auto tc = loplugin::TypeCheck(fieldDecl->getType());
if (tc.Class("map").StdNamespace() || tc.Class("unordered_map").StdNamespace())
continue;
// there is a loop in ~ImplPrnQueueList deleting stuff on a global data structure
if (loplugin::hasPathnamePrefix(aFileName, SRCDIR "/vcl/inc/print.h"))
return; return;
report( CheckDeleteExpr(destructorDecl, deleteExpr, memberExpr, "rather manage with std::some_container<std::unique_ptr<T>>");
DiagnosticsEngine::Warning,
"rather manage with std::some_container<std::unique_ptr<T>>",
deleteExpr->getLocStart())
<< deleteExpr->getSourceRange();
report(
DiagnosticsEngine::Note,
"member is here",
fieldDecl->getLocStart())
<< fieldDecl->getSourceRange();
}
} }
bool UseUniquePtr::VisitCompoundStmt(const CompoundStmt* compoundStmt) bool UseUniquePtr::VisitCompoundStmt(const CompoundStmt* compoundStmt)