[Libreoffice-commits] core.git: 2 commits - basic/source compilerplugins/clang
Libreoffice Gerrit user
logerrit at kemper.freedesktop.org
Wed Aug 22 06:26:51 UTC 2018
basic/source/comp/exprnode.cxx | 10
basic/source/comp/exprtree.cxx | 112 ++++----
basic/source/inc/expr.hxx | 32 +-
compilerplugins/clang/test/useuniqueptr.cxx | 50 +++
compilerplugins/clang/useuniqueptr.cxx | 358 ++++++++++++++++++++--------
5 files changed, 394 insertions(+), 168 deletions(-)
New commits:
commit a18a72ac1a71360b5d30c713437c8d2bd36321aa
Author: Noel Grandin <noel.grandin at collabora.co.uk>
AuthorDate: Mon Aug 20 08:35:18 2018 +0200
Commit: Noel Grandin <noel.grandin at collabora.co.uk>
CommitDate: Wed Aug 22 08:26:41 2018 +0200
loplugin:useuniqueptr in SbiExprNode
Change-Id: I2025251e35a48f47a51f11d790c3a22e118f3c05
Reviewed-on: https://gerrit.libreoffice.org/59348
Tested-by: Jenkins
Reviewed-by: Noel Grandin <noel.grandin at collabora.co.uk>
diff --git a/basic/source/comp/exprnode.cxx b/basic/source/comp/exprnode.cxx
index aa8f69828c37..ca341228d113 100644
--- a/basic/source/comp/exprnode.cxx
+++ b/basic/source/comp/exprnode.cxx
@@ -28,9 +28,9 @@
#include <expr.hxx>
-SbiExprNode::SbiExprNode( SbiExprNode* l, SbiToken t, SbiExprNode* r ) :
- pLeft(l),
- pRight(r),
+SbiExprNode::SbiExprNode( std::unique_ptr<SbiExprNode> l, SbiToken t, std::unique_ptr<SbiExprNode> r ) :
+ pLeft(std::move(l)),
+ pRight(std::move(r)),
pWithParent(nullptr),
eNodeType(SbxNODE),
eType(SbxVARIANT), // Nodes are always Variant
@@ -73,9 +73,9 @@ SbiExprNode::SbiExprNode( const SbiSymDef& r, SbxDataType t, SbiExprListPtr l )
}
// #120061 TypeOf
-SbiExprNode::SbiExprNode( SbiExprNode* l, sal_uInt16 nId ) :
+SbiExprNode::SbiExprNode( std::unique_ptr<SbiExprNode> l, sal_uInt16 nId ) :
nTypeStrId(nId),
- pLeft(l),
+ pLeft(std::move(l)),
pWithParent(nullptr),
eNodeType(SbxTYPEOF),
eType(SbxBOOL),
diff --git a/basic/source/comp/exprtree.cxx b/basic/source/comp/exprtree.cxx
index 229f415fc3c8..a8bae29ebbe5 100644
--- a/basic/source/comp/exprtree.cxx
+++ b/basic/source/comp/exprtree.cxx
@@ -32,7 +32,7 @@ SbiExpression::SbiExpression( SbiParser* p, SbiExprType t,
nParenLevel = 0;
eCurExpr = t;
m_eMode = eMode;
- pExpr.reset((t != SbSTDEXPR ) ? Term( pKeywordSymbolInfo ) : Boolean());
+ pExpr = (t != SbSTDEXPR ) ? Term( pKeywordSymbolInfo ) : Boolean();
if( t != SbSYMBOL )
{
pExpr->Optimize(pParser);
@@ -157,14 +157,14 @@ static SbiSymDef* AddSym ( SbiToken eTok, SbiSymPool& rPool, SbiExprType eCurExp
// currently even keywords are allowed (because of Dflt properties of the same name)
-SbiExprNode* SbiExpression::Term( const KeywordSymbolInfo* pKeywordSymbolInfo )
+std::unique_ptr<SbiExprNode> SbiExpression::Term( const KeywordSymbolInfo* pKeywordSymbolInfo )
{
if( pParser->Peek() == DOT )
{
SbiExprNode* pWithVar = pParser->GetWithVar();
// #26608: get to the node-chain's end to pass the correct object
SbiSymDef* pDef = pWithVar ? pWithVar->GetRealVar() : nullptr;
- SbiExprNode* pNd = nullptr;
+ std::unique_ptr<SbiExprNode> pNd;
if( !pDef )
{
pParser->Next();
@@ -180,7 +180,7 @@ SbiExprNode* SbiExpression::Term( const KeywordSymbolInfo* pKeywordSymbolInfo )
if( !pNd )
{
pParser->Error( ERRCODE_BASIC_UNEXPECTED, DOT );
- pNd = new SbiExprNode( 1.0, SbxDOUBLE );
+ pNd = o3tl::make_unique<SbiExprNode>( 1.0, SbxDOUBLE );
}
return pNd;
}
@@ -200,7 +200,7 @@ SbiExprNode* SbiExpression::Term( const KeywordSymbolInfo* pKeywordSymbolInfo )
if( eNextTok == ASSIGN )
{
pParser->UnlockColumn();
- return new SbiExprNode( aSym );
+ return o3tl::make_unique<SbiExprNode>( aSym );
}
// no keywords allowed from here on!
if( SbiTokenizer::IsKwd( eTok )
@@ -292,11 +292,11 @@ SbiExprNode* SbiExpression::Term( const KeywordSymbolInfo* pKeywordSymbolInfo )
delete pvMoreParLcl;
if( pConst->GetType() == SbxSTRING )
{
- return new SbiExprNode( pConst->GetString() );
+ return o3tl::make_unique<SbiExprNode>( pConst->GetString() );
}
else
{
- return new SbiExprNode( pConst->GetValue(), pConst->GetType() );
+ return o3tl::make_unique<SbiExprNode>( pConst->GetValue(), pConst->GetType() );
}
}
@@ -347,7 +347,7 @@ SbiExprNode* SbiExpression::Term( const KeywordSymbolInfo* pKeywordSymbolInfo )
}
}
}
- SbiExprNode* pNd = new SbiExprNode( *pDef, eType );
+ std::unique_ptr<SbiExprNode> pNd(new SbiExprNode( *pDef, eType ));
if( !pPar )
{
pPar = SbiExprList::ParseParameters( pParser,false,false );
@@ -372,7 +372,7 @@ SbiExprNode* SbiExpression::Term( const KeywordSymbolInfo* pKeywordSymbolInfo )
}
if( !bError )
{
- pNd->aVar.pNext = ObjTerm( *pDef );
+ pNd->aVar.pNext = ObjTerm( *pDef ).release();
}
}
@@ -383,7 +383,7 @@ SbiExprNode* SbiExpression::Term( const KeywordSymbolInfo* pKeywordSymbolInfo )
// construction of an object term. A term of this kind is part
// of an expression that begins with an object variable.
-SbiExprNode* SbiExpression::ObjTerm( SbiSymDef& rObj )
+std::unique_ptr<SbiExprNode> SbiExpression::ObjTerm( SbiSymDef& rObj )
{
pParser->Next();
SbiToken eTok = pParser->Next();
@@ -453,7 +453,7 @@ SbiExprNode* SbiExpression::ObjTerm( SbiSymDef& rObj )
pDef->SetType( eType );
}
- SbiExprNode* pNd = new SbiExprNode( *pDef, eType );
+ std::unique_ptr<SbiExprNode> pNd(new SbiExprNode( *pDef, eType ));
pNd->aVar.pPar = pPar.release();
pNd->aVar.pvMorePar = pvMoreParLcl;
if( bObj )
@@ -469,7 +469,7 @@ SbiExprNode* SbiExpression::ObjTerm( SbiSymDef& rObj )
}
if( !bError )
{
- pNd->aVar.pNext = ObjTerm( *pDef );
+ pNd->aVar.pNext = ObjTerm( *pDef ).release();
pNd->eType = eType;
}
}
@@ -484,9 +484,9 @@ SbiExprNode* SbiExpression::ObjTerm( SbiSymDef& rObj )
// functions
// bracketed expressions
-SbiExprNode* SbiExpression::Operand( bool bUsedForTypeOf )
+std::unique_ptr<SbiExprNode> SbiExpression::Operand( bool bUsedForTypeOf )
{
- SbiExprNode *pRes;
+ std::unique_ptr<SbiExprNode> pRes;
// test operand:
switch( SbiToken eTok = pParser->Peek() )
@@ -497,24 +497,24 @@ SbiExprNode* SbiExpression::Operand( bool bUsedForTypeOf )
if( !bUsedForTypeOf && pParser->IsVBASupportOn() && pParser->Peek() == IS )
{
eTok = pParser->Next();
- pRes = new SbiExprNode( pRes, eTok, Like() );
+ pRes = o3tl::make_unique<SbiExprNode>( std::move(pRes), eTok, Like() );
}
break;
case DOT: // .with
pRes = Term(); break;
case NUMBER:
pParser->Next();
- pRes = new SbiExprNode( pParser->GetDbl(), pParser->GetType() );
+ pRes = o3tl::make_unique<SbiExprNode>( pParser->GetDbl(), pParser->GetType() );
break;
case FIXSTRING:
pParser->Next();
- pRes = new SbiExprNode( pParser->GetSym() ); break;
+ pRes = o3tl::make_unique<SbiExprNode>( pParser->GetSym() ); break;
case LPAREN:
pParser->Next();
if( nParenLevel == 0 && m_eMode == EXPRMODE_LPAREN_PENDING && pParser->Peek() == RPAREN )
{
m_eMode = EXPRMODE_EMPTY_PAREN;
- pRes = new SbiExprNode(); // Dummy node
+ pRes = o3tl::make_unique<SbiExprNode>(); // Dummy node
pParser->Next();
break;
}
@@ -559,7 +559,7 @@ SbiExprNode* SbiExpression::Operand( bool bUsedForTypeOf )
else
{
pParser->Next();
- pRes = new SbiExprNode( 1.0, SbxDOUBLE );
+ pRes = o3tl::make_unique<SbiExprNode>( 1.0, SbxDOUBLE );
pParser->Error( ERRCODE_BASIC_UNEXPECTED, eTok );
}
break;
@@ -567,16 +567,16 @@ SbiExprNode* SbiExpression::Operand( bool bUsedForTypeOf )
return pRes;
}
-SbiExprNode* SbiExpression::Unary()
+std::unique_ptr<SbiExprNode> SbiExpression::Unary()
{
- SbiExprNode* pNd;
+ std::unique_ptr<SbiExprNode> pNd;
SbiToken eTok = pParser->Peek();
switch( eTok )
{
case MINUS:
eTok = NEG;
pParser->Next();
- pNd = new SbiExprNode( Unary(), eTok, nullptr );
+ pNd = o3tl::make_unique<SbiExprNode>( Unary(), eTok, nullptr );
break;
case NOT:
if( pParser->IsVBASupportOn() )
@@ -586,7 +586,7 @@ SbiExprNode* SbiExpression::Unary()
else
{
pParser->Next();
- pNd = new SbiExprNode( Unary(), eTok, nullptr );
+ pNd = o3tl::make_unique<SbiExprNode>( Unary(), eTok, nullptr );
}
break;
case PLUS:
@@ -596,11 +596,11 @@ SbiExprNode* SbiExpression::Unary()
case TYPEOF:
{
pParser->Next();
- SbiExprNode* pObjNode = Operand( true/*bUsedForTypeOf*/ );
+ std::unique_ptr<SbiExprNode> pObjNode = Operand( true/*bUsedForTypeOf*/ );
pParser->TestToken( IS );
SbiSymDef* pTypeDef = new SbiSymDef( OUString() );
pParser->TypeDecl( *pTypeDef, true );
- pNd = new SbiExprNode( pObjNode, pTypeDef->GetTypeId() );
+ pNd = o3tl::make_unique<SbiExprNode>( std::move(pObjNode), pTypeDef->GetTypeId() );
break;
}
case NEW:
@@ -608,7 +608,7 @@ SbiExprNode* SbiExpression::Unary()
pParser->Next();
SbiSymDef* pTypeDef = new SbiSymDef( OUString() );
pParser->TypeDecl( *pTypeDef, true );
- pNd = new SbiExprNode( pTypeDef->GetTypeId() );
+ pNd = o3tl::make_unique<SbiExprNode>( pTypeDef->GetTypeId() );
break;
}
default:
@@ -617,23 +617,23 @@ SbiExprNode* SbiExpression::Unary()
return pNd;
}
-SbiExprNode* SbiExpression::Exp()
+std::unique_ptr<SbiExprNode> SbiExpression::Exp()
{
- SbiExprNode* pNd = Unary();
+ std::unique_ptr<SbiExprNode> pNd = Unary();
if( m_eMode != EXPRMODE_EMPTY_PAREN )
{
while( pParser->Peek() == EXPON )
{
SbiToken eTok = pParser->Next();
- pNd = new SbiExprNode( pNd, eTok, Unary() );
+ pNd = o3tl::make_unique<SbiExprNode>( std::move(pNd), eTok, Unary() );
}
}
return pNd;
}
-SbiExprNode* SbiExpression::MulDiv()
+std::unique_ptr<SbiExprNode> SbiExpression::MulDiv()
{
- SbiExprNode* pNd = Exp();
+ std::unique_ptr<SbiExprNode> pNd = Exp();
if( m_eMode != EXPRMODE_EMPTY_PAREN )
{
for( ;; )
@@ -644,43 +644,43 @@ SbiExprNode* SbiExpression::MulDiv()
break;
}
eTok = pParser->Next();
- pNd = new SbiExprNode( pNd, eTok, Exp() );
+ pNd = o3tl::make_unique<SbiExprNode>( std::move(pNd), eTok, Exp() );
}
}
return pNd;
}
-SbiExprNode* SbiExpression::IntDiv()
+std::unique_ptr<SbiExprNode> SbiExpression::IntDiv()
{
- SbiExprNode* pNd = MulDiv();
+ std::unique_ptr<SbiExprNode> pNd = MulDiv();
if( m_eMode != EXPRMODE_EMPTY_PAREN )
{
while( pParser->Peek() == IDIV )
{
SbiToken eTok = pParser->Next();
- pNd = new SbiExprNode( pNd, eTok, MulDiv() );
+ pNd = o3tl::make_unique<SbiExprNode>( std::move(pNd), eTok, MulDiv() );
}
}
return pNd;
}
-SbiExprNode* SbiExpression::Mod()
+std::unique_ptr<SbiExprNode> SbiExpression::Mod()
{
- SbiExprNode* pNd = IntDiv();
+ std::unique_ptr<SbiExprNode> pNd = IntDiv();
if( m_eMode != EXPRMODE_EMPTY_PAREN )
{
while( pParser->Peek() == MOD )
{
SbiToken eTok = pParser->Next();
- pNd = new SbiExprNode( pNd, eTok, IntDiv() );
+ pNd = o3tl::make_unique<SbiExprNode>( std::move(pNd), eTok, IntDiv() );
}
}
return pNd;
}
-SbiExprNode* SbiExpression::AddSub()
+std::unique_ptr<SbiExprNode> SbiExpression::AddSub()
{
- SbiExprNode* pNd = Mod();
+ std::unique_ptr<SbiExprNode> pNd = Mod();
if( m_eMode != EXPRMODE_EMPTY_PAREN )
{
for( ;; )
@@ -691,15 +691,15 @@ SbiExprNode* SbiExpression::AddSub()
break;
}
eTok = pParser->Next();
- pNd = new SbiExprNode( pNd, eTok, Mod() );
+ pNd = o3tl::make_unique<SbiExprNode>( std::move(pNd), eTok, Mod() );
}
}
return pNd;
}
-SbiExprNode* SbiExpression::Cat()
+std::unique_ptr<SbiExprNode> SbiExpression::Cat()
{
- SbiExprNode* pNd = AddSub();
+ std::unique_ptr<SbiExprNode> pNd = AddSub();
if( m_eMode != EXPRMODE_EMPTY_PAREN )
{
for( ;; )
@@ -710,15 +710,15 @@ SbiExprNode* SbiExpression::Cat()
break;
}
eTok = pParser->Next();
- pNd = new SbiExprNode( pNd, eTok, AddSub() );
+ pNd = o3tl::make_unique<SbiExprNode>( std::move(pNd), eTok, AddSub() );
}
}
return pNd;
}
-SbiExprNode* SbiExpression::Comp()
+std::unique_ptr<SbiExprNode> SbiExpression::Comp()
{
- SbiExprNode* pNd = Cat();
+ std::unique_ptr<SbiExprNode> pNd = Cat();
if( m_eMode != EXPRMODE_EMPTY_PAREN )
{
short nCount = 0;
@@ -735,7 +735,7 @@ SbiExprNode* SbiExpression::Comp()
break;
}
eTok = pParser->Next();
- pNd = new SbiExprNode( pNd, eTok, Cat() );
+ pNd = o3tl::make_unique<SbiExprNode>( std::move(pNd), eTok, Cat() );
nCount++;
}
}
@@ -743,15 +743,15 @@ SbiExprNode* SbiExpression::Comp()
}
-SbiExprNode* SbiExpression::VBA_Not()
+std::unique_ptr<SbiExprNode> SbiExpression::VBA_Not()
{
- SbiExprNode* pNd = nullptr;
+ std::unique_ptr<SbiExprNode> pNd;
SbiToken eTok = pParser->Peek();
if( eTok == NOT )
{
pParser->Next();
- pNd = new SbiExprNode( VBA_Not(), eTok, nullptr );
+ pNd = o3tl::make_unique<SbiExprNode>( VBA_Not(), eTok, nullptr );
}
else
{
@@ -760,16 +760,16 @@ SbiExprNode* SbiExpression::VBA_Not()
return pNd;
}
-SbiExprNode* SbiExpression::Like()
+std::unique_ptr<SbiExprNode> SbiExpression::Like()
{
- SbiExprNode* pNd = pParser->IsVBASupportOn() ? VBA_Not() : Comp();
+ std::unique_ptr<SbiExprNode> pNd = pParser->IsVBASupportOn() ? VBA_Not() : Comp();
if( m_eMode != EXPRMODE_EMPTY_PAREN )
{
short nCount = 0;
while( pParser->Peek() == LIKE )
{
SbiToken eTok = pParser->Next();
- pNd = new SbiExprNode( pNd, eTok, Comp() );
+ pNd = o3tl::make_unique<SbiExprNode>( std::move(pNd), eTok, Comp() );
nCount++;
}
// multiple operands in a row does not work
@@ -782,9 +782,9 @@ SbiExprNode* SbiExpression::Like()
return pNd;
}
-SbiExprNode* SbiExpression::Boolean()
+std::unique_ptr<SbiExprNode> SbiExpression::Boolean()
{
- SbiExprNode* pNd = Like();
+ std::unique_ptr<SbiExprNode> pNd = Like();
if( m_eMode != EXPRMODE_EMPTY_PAREN )
{
for( ;; )
@@ -797,7 +797,7 @@ SbiExprNode* SbiExpression::Boolean()
break;
}
eTok = pParser->Next();
- pNd = new SbiExprNode( pNd, eTok, Like() );
+ pNd = o3tl::make_unique<SbiExprNode>( std::move(pNd), eTok, Like() );
}
}
return pNd;
diff --git a/basic/source/inc/expr.hxx b/basic/source/inc/expr.hxx
index 0751e7de4c21..e0b088ef15d8 100644
--- a/basic/source/inc/expr.hxx
+++ b/basic/source/inc/expr.hxx
@@ -117,8 +117,8 @@ public:
SbiExprNode( double, SbxDataType );
SbiExprNode( const OUString& );
SbiExprNode( const SbiSymDef&, SbxDataType, SbiExprListPtr = nullptr );
- SbiExprNode( SbiExprNode*, SbiToken, SbiExprNode* );
- SbiExprNode( SbiExprNode*, sal_uInt16 ); // #120061 TypeOf
+ SbiExprNode( std::unique_ptr<SbiExprNode>, SbiToken, std::unique_ptr<SbiExprNode> );
+ SbiExprNode( std::unique_ptr<SbiExprNode>, sal_uInt16 ); // #120061 TypeOf
SbiExprNode( sal_uInt16 ); // new <type>
~SbiExprNode();
@@ -158,20 +158,20 @@ protected:
bool bByVal; // true: ByVal-Parameter
bool bBracket; // true: Parameter list with brackets
sal_uInt16 nParenLevel;
- SbiExprNode* Term( const KeywordSymbolInfo* pKeywordSymbolInfo = nullptr );
- SbiExprNode* ObjTerm( SbiSymDef& );
- SbiExprNode* Operand( bool bUsedForTypeOf = false );
- SbiExprNode* Unary();
- SbiExprNode* Exp();
- SbiExprNode* MulDiv();
- SbiExprNode* IntDiv();
- SbiExprNode* Mod();
- SbiExprNode* AddSub();
- SbiExprNode* Cat();
- SbiExprNode* Like();
- SbiExprNode* VBA_Not();
- SbiExprNode* Comp();
- SbiExprNode* Boolean();
+ std::unique_ptr<SbiExprNode> Term( const KeywordSymbolInfo* pKeywordSymbolInfo = nullptr );
+ std::unique_ptr<SbiExprNode> ObjTerm( SbiSymDef& );
+ std::unique_ptr<SbiExprNode> Operand( bool bUsedForTypeOf = false );
+ std::unique_ptr<SbiExprNode> Unary();
+ std::unique_ptr<SbiExprNode> Exp();
+ std::unique_ptr<SbiExprNode> MulDiv();
+ std::unique_ptr<SbiExprNode> IntDiv();
+ std::unique_ptr<SbiExprNode> Mod();
+ std::unique_ptr<SbiExprNode> AddSub();
+ std::unique_ptr<SbiExprNode> Cat();
+ std::unique_ptr<SbiExprNode> Like();
+ std::unique_ptr<SbiExprNode> VBA_Not();
+ std::unique_ptr<SbiExprNode> Comp();
+ std::unique_ptr<SbiExprNode> Boolean();
public:
SbiExpression( SbiParser*, SbiExprType = SbSTDEXPR,
SbiExprMode eMode = EXPRMODE_STANDARD, const KeywordSymbolInfo* pKeywordSymbolInfo = nullptr ); // parsing Ctor
commit 86b2f0ad773feb42ee0a308d5e0152d4e53f1ee5
Author: Noel Grandin <noel.grandin at collabora.co.uk>
AuthorDate: Thu Aug 16 13:46:39 2018 +0200
Commit: Noel Grandin <noel.grandin at collabora.co.uk>
CommitDate: Wed Aug 22 08:26:30 2018 +0200
improvements to loplugin:useuniqueptr
find more patterns of deleting member data
Change-Id: I8913e364200dad8405bac30dd8cab2a68c8bee8f
Reviewed-on: https://gerrit.libreoffice.org/59233
Tested-by: Jenkins
Reviewed-by: Noel Grandin <noel.grandin at collabora.co.uk>
diff --git a/compilerplugins/clang/test/useuniqueptr.cxx b/compilerplugins/clang/test/useuniqueptr.cxx
index 7b9870b6f6bf..020bcce1981e 100644
--- a/compilerplugins/clang/test/useuniqueptr.cxx
+++ b/compilerplugins/clang/test/useuniqueptr.cxx
@@ -7,6 +7,8 @@
* file, You can obtain one at http://mozilla.org/MPL/2.0/.
*/
+#include <sal/config.h>
+#include <config_clang.h>
#include <array>
#include <memory>
#include <vector>
@@ -62,6 +64,18 @@ class Class5 {
delete p; // expected-error {{rather manage with std::some_container<std::unique_ptr<T>> [loplugin:useuniqueptr]}}
}
};
+class Class5a {
+ int* m_pbar[10]; // expected-note {{member is here [loplugin:useuniqueptr]}}
+ ~Class5a()
+ {
+ for (auto p : m_pbar)
+ {
+ int x = 1;
+ x = x + 2;
+ delete p; // expected-error {{rather manage with std::some_container<std::unique_ptr<T>> [loplugin:useuniqueptr]}}
+ }
+ }
+};
class Class6 {
std::array<int*,10> m_pbar; // expected-note {{member is here [loplugin:useuniqueptr]}}
~Class6()
@@ -167,6 +181,7 @@ class Foo13 {
}
}
};
+
// check for unconditional inner compound statements
class Foo14 {
int * m_pbar1; // expected-note {{member is here [loplugin:useuniqueptr]}}
@@ -177,10 +192,43 @@ class Foo14 {
}
}
};
+
void Foo15(int * p)
{
- delete p; // expected-error {{calling delete on a pointer param, should be either whitelisted here or simplified [loplugin:useuniqueptr]}}
+ delete p; // expected-error {{calling delete on a pointer param, should be either whitelisted or simplified [loplugin:useuniqueptr]}}
+};
+
+class Foo16 {
+ Foo16(int * p)
+ {
+ delete p; // expected-error {{calling delete on a pointer param, should be either whitelisted or simplified [loplugin:useuniqueptr]}}
+ };
+ void foo(int * p)
+ {
+ delete p; // expected-error {{calling delete on a pointer param, should be either whitelisted or simplified [loplugin:useuniqueptr]}}
+ };
+};
+
+// check for delete on array members
+class Foo17 {
+ int * m_pbar1[6]; // expected-note {{member is here [loplugin:useuniqueptr]}}
+ ~Foo17()
+ {
+ delete m_pbar1[0]; // expected-error {{unconditional call to delete on a member, should be using std::unique_ptr [loplugin:useuniqueptr]}}
+ }
+};
+
+// this only starts to work somewhere after clang 3.8 and before clang7
+#if CLANG_VERSION >= 30900
+class Foo18 {
+ std::vector<char*> m_pbar1; // expected-note {{member is here [loplugin:useuniqueptr]}}
+ ~Foo18()
+ {
+ for (auto aIter = m_pbar1.begin(); aIter != m_pbar1.end(); ++aIter)
+ delete *aIter; // expected-error {{rather manage with std::some_container<std::unique_ptr<T>> [loplugin:useuniqueptr]}}
+ }
};
+#endif
// ------------------------------------------------------------------------------------------------
// tests for passing owning pointers to constructors
diff --git a/compilerplugins/clang/useuniqueptr.cxx b/compilerplugins/clang/useuniqueptr.cxx
index 1192513a17d0..c13e12186c49 100644
--- a/compilerplugins/clang/useuniqueptr.cxx
+++ b/compilerplugins/clang/useuniqueptr.cxx
@@ -124,13 +124,14 @@ public:
bool VisitCompoundStmt(const CompoundStmt* );
bool VisitCXXDeleteExpr(const CXXDeleteExpr* );
bool TraverseFunctionDecl(FunctionDecl* );
+ bool TraverseCXXMethodDecl(CXXMethodDecl* );
+ bool TraverseCXXConstructorDecl(CXXConstructorDecl* );
bool TraverseConstructorInitializer(CXXCtorInitializer*);
private:
void CheckCompoundStmt(const CXXMethodDecl*, const CompoundStmt* );
- void CheckForUnconditionalDelete(const CXXMethodDecl*, const CompoundStmt* );
- void CheckForSimpleDelete(const CXXMethodDecl*, const CompoundStmt* );
- void CheckRangedLoopDelete(const CXXMethodDecl*, const CXXForRangeStmt* );
+ void CheckIfStmt(const CXXMethodDecl* methodDecl, const IfStmt* );
+ void CheckCXXForRangeStmt(const CXXMethodDecl*, const CXXForRangeStmt* );
void CheckLoopDelete(const CXXMethodDecl*, const Stmt* );
void CheckLoopDelete(const CXXMethodDecl*, const CXXDeleteExpr* );
void CheckDeleteExpr(const CXXMethodDecl*, const CXXDeleteExpr*);
@@ -156,14 +157,15 @@ bool UseUniquePtr::VisitCXXMethodDecl(const CXXMethodDecl* methodDecl)
return true;
}
+/**
+ * check for simple call to delete i.e. direct unconditional call, or if-guarded call
+ */
void UseUniquePtr::CheckCompoundStmt(const CXXMethodDecl* methodDecl, const CompoundStmt* compoundStmt)
{
- CheckForSimpleDelete(methodDecl, compoundStmt);
-
for (auto i = compoundStmt->body_begin(); i != compoundStmt->body_end(); ++i)
{
if (auto cxxForRangeStmt = dyn_cast<CXXForRangeStmt>(*i))
- CheckRangedLoopDelete(methodDecl, cxxForRangeStmt);
+ CheckCXXForRangeStmt(methodDecl, cxxForRangeStmt);
else if (auto forStmt = dyn_cast<ForStmt>(*i))
CheckLoopDelete(methodDecl, forStmt->getBody());
else if (auto whileStmt = dyn_cast<WhileStmt>(*i))
@@ -171,94 +173,86 @@ void UseUniquePtr::CheckCompoundStmt(const CXXMethodDecl* methodDecl, const Comp
// check for unconditional inner compound statements
else if (auto innerCompoundStmt = dyn_cast<CompoundStmt>(*i))
CheckCompoundStmt(methodDecl, innerCompoundStmt);
+ else if (auto deleteExpr = dyn_cast<CXXDeleteExpr>(*i))
+ CheckDeleteExpr(methodDecl, deleteExpr);
+ else if (auto parenExpr = dyn_cast<ParenExpr>(*i))
+ CheckParenExpr(methodDecl, parenExpr);
+ else if (auto ifStmt = dyn_cast<IfStmt>(*i))
+ CheckIfStmt(methodDecl, ifStmt);
}
}
-/**
- * check for simple call to delete in a destructor i.e. direct unconditional call, or if-guarded call
- */
-void UseUniquePtr::CheckForSimpleDelete(const CXXMethodDecl* methodDecl, const CompoundStmt* compoundStmt)
+// Check for conditional deletes like:
+// if (m_pField != nullptr) delete m_pField;
+void UseUniquePtr::CheckIfStmt(const CXXMethodDecl* methodDecl, const IfStmt* ifStmt)
{
- for (auto i = compoundStmt->body_begin(); i != compoundStmt->body_end(); ++i)
+ auto cond = ifStmt->getCond()->IgnoreImpCasts();
+ if (auto ifCondMemberExpr = dyn_cast<MemberExpr>(cond))
{
- auto deleteExpr = dyn_cast<CXXDeleteExpr>(*i);
- if (deleteExpr)
- {
- CheckDeleteExpr(methodDecl, deleteExpr);
- continue;
- }
- auto parenExpr = dyn_cast<ParenExpr>(*i);
- if (parenExpr)
- {
- CheckParenExpr(methodDecl, parenExpr);
- continue;
- }
- // Check for conditional deletes like:
- // if (m_pField != nullptr) delete m_pField;
- auto ifStmt = dyn_cast<IfStmt>(*i);
- if (!ifStmt)
- continue;
- auto cond = ifStmt->getCond()->IgnoreImpCasts();
- if (auto ifCondMemberExpr = dyn_cast<MemberExpr>(cond))
- {
- // ignore "if (bMine)"
- if (!loplugin::TypeCheck(ifCondMemberExpr->getType()).Pointer())
- continue;
- // good
- }
- else if (auto binaryOp = dyn_cast<BinaryOperator>(cond))
- {
- if (!isa<MemberExpr>(binaryOp->getLHS()->IgnoreImpCasts()))
- continue;
- if (!isa<CXXNullPtrLiteralExpr>(binaryOp->getRHS()->IgnoreImpCasts()))
- continue;
- // good
- }
- else // ignore anything more complicated
- continue;
+ // ignore "if (bMine)"
+ if (!loplugin::TypeCheck(ifCondMemberExpr->getType()).Pointer())
+ return;
+ // good
+ }
+ else if (auto binaryOp = dyn_cast<BinaryOperator>(cond))
+ {
+ if (!isa<MemberExpr>(binaryOp->getLHS()->IgnoreImpCasts()))
+ return;
+ if (!isa<CXXNullPtrLiteralExpr>(binaryOp->getRHS()->IgnoreImpCasts()))
+ return;
+ // good
+ }
+ else // ignore anything more complicated
+ return;
- deleteExpr = dyn_cast<CXXDeleteExpr>(ifStmt->getThen());
- if (deleteExpr)
- {
- CheckDeleteExpr(methodDecl, deleteExpr);
- continue;
- }
+ auto deleteExpr = dyn_cast<CXXDeleteExpr>(ifStmt->getThen());
+ if (deleteExpr)
+ {
+ CheckDeleteExpr(methodDecl, deleteExpr);
+ return;
+ }
- parenExpr = dyn_cast<ParenExpr>(ifStmt->getThen());
+ auto parenExpr = dyn_cast<ParenExpr>(ifStmt->getThen());
+ if (parenExpr)
+ {
+ CheckParenExpr(methodDecl, parenExpr);
+ return;
+ }
+
+ auto ifThenCompoundStmt = dyn_cast<CompoundStmt>(ifStmt->getThen());
+ if (!ifThenCompoundStmt)
+ return;
+ for (auto j = ifThenCompoundStmt->body_begin(); j != ifThenCompoundStmt->body_end(); ++j)
+ {
+ auto ifDeleteExpr = dyn_cast<CXXDeleteExpr>(*j);
+ if (ifDeleteExpr)
+ CheckDeleteExpr(methodDecl, ifDeleteExpr);
+ ParenExpr const * parenExpr = dyn_cast<ParenExpr>(*j);
if (parenExpr)
- {
CheckParenExpr(methodDecl, parenExpr);
- continue;
- }
-
- auto ifThenCompoundStmt = dyn_cast<CompoundStmt>(ifStmt->getThen());
- if (!ifThenCompoundStmt)
- continue;
- for (auto j = ifThenCompoundStmt->body_begin(); j != ifThenCompoundStmt->body_end(); ++j)
- {
- auto ifDeleteExpr = dyn_cast<CXXDeleteExpr>(*j);
- if (ifDeleteExpr)
- CheckDeleteExpr(methodDecl, ifDeleteExpr);
- ParenExpr const * parenExpr = dyn_cast<ParenExpr>(*j);
- if (parenExpr)
- CheckParenExpr(methodDecl, parenExpr);
- }
}
}
-/**
- * Check the delete expression in a destructor.
- */
void UseUniquePtr::CheckDeleteExpr(const CXXMethodDecl* methodDecl, const CXXDeleteExpr* deleteExpr)
{
- const ImplicitCastExpr* castExpr = dyn_cast<ImplicitCastExpr>(deleteExpr->getArgument());
- if (!castExpr)
- return;
- const MemberExpr* memberExpr = dyn_cast<MemberExpr>(castExpr->getSubExpr());
- if (!memberExpr)
+ auto deleteExprArg = deleteExpr->getArgument()->IgnoreParenImpCasts();
+
+ const MemberExpr* memberExpr = dyn_cast<MemberExpr>(deleteExprArg);
+ if (memberExpr)
+ {
+ CheckDeleteExpr(methodDecl, deleteExpr, memberExpr,
+ "unconditional call to delete on a member, should be using std::unique_ptr");
return;
- CheckDeleteExpr(methodDecl, deleteExpr, memberExpr,
- "unconditional call to delete on a member, should be using std::unique_ptr");
+ }
+
+ const ArraySubscriptExpr* arrayExpr = dyn_cast<ArraySubscriptExpr>(deleteExprArg);
+ if (arrayExpr)
+ {
+ auto baseMemberExpr = dyn_cast<MemberExpr>(arrayExpr->getBase()->IgnoreParenImpCasts());
+ if (baseMemberExpr)
+ CheckDeleteExpr(methodDecl, deleteExpr, baseMemberExpr,
+ "unconditional call to delete on a member, should be using std::unique_ptr");
+ }
}
/**
@@ -275,9 +269,6 @@ void UseUniquePtr::CheckParenExpr(const CXXMethodDecl* methodDecl, const ParenEx
CheckDeleteExpr(methodDecl, deleteExpr);
}
-/**
- * Check the delete expression in a destructor.
- */
void UseUniquePtr::CheckDeleteExpr(const CXXMethodDecl* methodDecl, const CXXDeleteExpr* deleteExpr,
const MemberExpr* memberExpr, StringRef message)
{
@@ -380,6 +371,7 @@ void UseUniquePtr::CheckLoopDelete(const CXXMethodDecl* methodDecl, const CXXDel
{
const MemberExpr* memberExpr = nullptr;
const Expr* subExpr = deleteExpr->getArgument();
+ // drill down looking for a MemberExpr
for (;;)
{
subExpr = subExpr->IgnoreImpCasts();
@@ -389,10 +381,26 @@ void UseUniquePtr::CheckLoopDelete(const CXXMethodDecl* methodDecl, const CXXDel
subExpr = arraySubscriptExpr->getBase();
else if (auto cxxOperatorCallExpr = dyn_cast<CXXOperatorCallExpr>(subExpr))
{
+ // look for deletes of an iterator object where the iterator is over a member field
+ if (auto declRefExpr = dyn_cast<DeclRefExpr>(cxxOperatorCallExpr->getArg(0)->IgnoreImpCasts()))
+ {
+ if (auto varDecl = dyn_cast<VarDecl>(declRefExpr->getDecl()))
+ {
+ auto init = varDecl->getInit();
+ if (auto x = dyn_cast<ExprWithCleanups>(init))
+ init = x->getSubExpr();
+ if (auto x = dyn_cast<CXXBindTemporaryExpr>(init))
+ init = x->getSubExpr();
+ if (auto x = dyn_cast<CXXMemberCallExpr>(init))
+ init = x->getImplicitObjectArgument();
+ memberExpr = dyn_cast<MemberExpr>(init);
+ break;
+ }
+ }
+ // look for deletes like "delete m_pField[0]"
if (cxxOperatorCallExpr->getOperator() == OO_Subscript)
{
memberExpr = dyn_cast<MemberExpr>(cxxOperatorCallExpr->getArg(0));
- break;
}
break;
}
@@ -405,11 +413,15 @@ void UseUniquePtr::CheckLoopDelete(const CXXMethodDecl* methodDecl, const CXXDel
CheckDeleteExpr(methodDecl, deleteExpr, memberExpr, "rather manage with std::some_container<std::unique_ptr<T>>");
}
-void UseUniquePtr::CheckRangedLoopDelete(const CXXMethodDecl* methodDecl, const CXXForRangeStmt* cxxForRangeStmt)
+void UseUniquePtr::CheckCXXForRangeStmt(const CXXMethodDecl* methodDecl, const CXXForRangeStmt* cxxForRangeStmt)
{
CXXDeleteExpr const * deleteExpr = nullptr;
if (auto compoundStmt = dyn_cast<CompoundStmt>(cxxForRangeStmt->getBody()))
- deleteExpr = dyn_cast<CXXDeleteExpr>(*compoundStmt->body_begin());
+ {
+ for (auto i = compoundStmt->body_begin(); i != compoundStmt->body_end(); ++i)
+ if ((deleteExpr = dyn_cast<CXXDeleteExpr>(*i)))
+ break;
+ }
else
deleteExpr = dyn_cast<CXXDeleteExpr>(cxxForRangeStmt->getBody());
if (!deleteExpr)
@@ -444,10 +456,7 @@ bool UseUniquePtr::VisitCompoundStmt(const CompoundStmt* compoundStmt)
return true;
}
- auto pCastExpr = dyn_cast<ImplicitCastExpr>(deleteExpr->getArgument());
- if (!pCastExpr)
- return true;
- auto declRefExpr = dyn_cast<DeclRefExpr>(pCastExpr->getSubExpr());
+ auto declRefExpr = dyn_cast<DeclRefExpr>(deleteExpr->getArgument()->IgnoreParenImpCasts());
if (!declRefExpr)
return true;
auto varDecl = dyn_cast<VarDecl>(declRefExpr->getDecl());
@@ -488,6 +497,32 @@ bool UseUniquePtr::TraverseFunctionDecl(FunctionDecl* functionDecl)
return ret;
}
+bool UseUniquePtr::TraverseCXXMethodDecl(CXXMethodDecl* methodDecl)
+{
+ if (ignoreLocation(methodDecl))
+ return true;
+
+ auto oldCurrent = mpCurrentFunctionDecl;
+ mpCurrentFunctionDecl = methodDecl;
+ bool ret = RecursiveASTVisitor::TraverseCXXMethodDecl(methodDecl);
+ mpCurrentFunctionDecl = oldCurrent;
+
+ return ret;
+}
+
+bool UseUniquePtr::TraverseCXXConstructorDecl(CXXConstructorDecl* methodDecl)
+{
+ if (ignoreLocation(methodDecl))
+ return true;
+
+ auto oldCurrent = mpCurrentFunctionDecl;
+ mpCurrentFunctionDecl = methodDecl;
+ bool ret = RecursiveASTVisitor::TraverseCXXConstructorDecl(methodDecl);
+ mpCurrentFunctionDecl = oldCurrent;
+
+ return ret;
+}
+
bool UseUniquePtr::VisitCXXDeleteExpr(const CXXDeleteExpr* deleteExpr)
{
if (!mpCurrentFunctionDecl)
@@ -514,6 +549,22 @@ bool UseUniquePtr::VisitCXXDeleteExpr(const CXXDeleteExpr* deleteExpr)
|| name == "lcl_MergeGCBox" || name == "lcl_MergeGCLine" || name == "lcl_DelHFFormat")
return true;
}
+ if (auto cxxMethodDecl = dyn_cast<CXXMethodDecl>(mpCurrentFunctionDecl))
+ {
+ // include/o3tl/deleter.hxx
+ if (cxxMethodDecl->getParent()->getName() == "default_delete")
+ return true;
+ // TODO Bitmap::ReleaseAccess
+ // Tricky because it reverberates through other code and requires that BitmapWriteAccess move into /include again
+ if (cxxMethodDecl->getParent()->getName() == "Bitmap")
+ return true;
+ // TODO virtual ones are much trickier, leave for later
+ if (cxxMethodDecl->isVirtual())
+ return true;
+ // sw/inc/unobaseclass.hxx holds SolarMutex while deleting
+ if (cxxMethodDecl->getParent()->getName() == "UnoImplPtrDeleter")
+ return true;
+ }
auto declRefExpr = dyn_cast<DeclRefExpr>(deleteExpr->getArgument()->IgnoreParenImpCasts());
if (!declRefExpr)
@@ -522,13 +573,140 @@ bool UseUniquePtr::VisitCXXDeleteExpr(const CXXDeleteExpr* deleteExpr)
if (!varDecl)
return true;
+ std::string fn(handler.getMainFileName());
+ loplugin::normalizeDotDotInFilePath(fn);
+
+ // StgAvlNode::Remove
+ if (fn == SRCDIR "/sot/source/sdstor/stgavl.cxx")
+ return true;
+ // SfxItemPool::ReleaseDefaults and SfxItemPool::Free
+ if (fn == SRCDIR "/svl/source/items/itempool.cxx")
+ return true;
+ // SwContourCache
+ if (fn == SRCDIR "/sw/source/core/text/txtfly.cxx")
+ return true;
+ // too messy to cope with the SQL parser
+ if (fn == SRCDIR "/connectivity/source/parse/sqlnode.cxx")
+ return true;
+ // I can't figure out the ownership of the SfxMedium in the call site(s)
+ if (fn == SRCDIR "/sfx2/source/doc/sfxbasemodel.cxx")
+ return true;
+ // pointer passed via IMPL_LINK
+ if (fn == SRCDIR "/sfx2/source/control/dispatch.cxx")
+ return true;
+ // NavigatorTreeModel::Remove
+ if (fn == SRCDIR "/svx/source/form/navigatortreemodel.cxx")
+ return true;
+ // SdrModel::AddUndo
+ if (fn == SRCDIR "/svx/source/svdraw/svdmodel.cxx")
+ return true;
+ // undo callback
+ if (fn == SRCDIR "/basctl/source/basicide/baside3.cxx")
+ return true;
+ // ActualizeProgress::TimeoutHdl
+ if (fn == SRCDIR "/cui/source/dialogs/cuigaldlg.cxx")
+ return true;
+ // ToolbarSaveInData::RemoveToolbar
+ if (fn == SRCDIR "/cui/source/customize/cfg.cxx")
+ return true;
+ // OStorage_Impl::RemoveElement very complicated ownership passing going on
+ if (fn == SRCDIR "/package/source/xstor/xstorage.cxx")
+ return true;
+ // actually held via shared_ptr, uses protected deleter object
+ if (fn == SRCDIR "/sd/source/ui/framework/tools/FrameworkHelper.cxx")
+ return true;
+ // actually held via shared_ptr, uses protected deleter object
+ if (fn == SRCDIR "/sd/source/ui/presenter/CanvasUpdateRequester.cxx")
+ return true;
+ // actually held via shared_ptr, uses protected deleter object
+ if (fn == SRCDIR "/sd/source/ui/slidesorter/cache/SlsPageCacheManager.cxx")
+ return true;
+ // actually held via shared_ptr, uses protected deleter object
+ if (fn == SRCDIR "/sd/source/ui/sidebar/MasterPageContainer.cxx")
+ return true;
+ // actually held via shared_ptr, uses protected deleter object
+ if (fn == SRCDIR "/sd/source/ui/tools/TimerBasedTaskExecution.cxx")
+ return true;
+ // actually held via shared_ptr, uses protected deleter object
+ if (fn == SRCDIR "/sd/source/ui/view/ViewShellImplementation.cxx")
+ return true;
+ // ScBroadcastAreaSlot::StartListeningArea manual ref-counting of ScBroadcastArea
+ if (fn == SRCDIR "/sc/source/core/data/bcaslot.cxx")
+ return true;
+ // ScDrawLayer::AddCalcUndo undo stuff
+ if (fn == SRCDIR "/sc/source/core/data/drwlayer.cxx")
+ return true;
+ // ScTable::SetFormulaCell
+ if (fn == SRCDIR "/sc/source/core/data/table2.cxx")
+ return true;
+ // ScDocument::SetFormulaCell
+ if (fn == SRCDIR "/sc/source/core/data/documen2.cxx")
+ return true;
+ // RemoveEditAttribsHandler, stored in mdds block
+ if (fn == SRCDIR "/sc/source/core/data/column2.cxx")
+ return true;
+ // just turns into a mess
+ if (fn == SRCDIR "/sc/source/ui/Accessibility/AccessibleDocument.cxx")
+ return true;
+ // SwCache::DeleteObj, linked list
+ if (fn == SRCDIR "/sw/source/core/bastyp/swcache.cxx")
+ return true;
+ // SAXEventKeeperImpl::smashBufferNode
+ if (fn == SRCDIR "/xmlsecurity/source/framework/saxeventkeeperimpl.cxx")
+ return true;
+ // ZipOutputStream, ownership of ZipEntry is horribly complicated here
+ if (fn == SRCDIR "/package/source/zipapi/ZipOutputStream.cxx")
+ return true;
+ // SwDoc::DeleteExtTextInput
+ if (fn == SRCDIR "/sw/source/core/doc/extinput.cxx")
+ return true;
+ // SwDoc::DelSectionFormat
+ if (fn == SRCDIR "/sw/source/core/docnode/ndsect.cxx")
+ return true;
+ // SwFrame::DestroyFrame
+ if (fn == SRCDIR "/sw/source/core/layout/ssfrm.cxx")
+ return true;
+ // SwGluePortion::Join
+ if (fn == SRCDIR "/sw/source/core/text/porglue.cxx")
+ return true;
+ // SwDoc::DelFrameFormat
+ if (fn == SRCDIR "/sw/source/core/doc/docfmt.cxx")
+ return true;
+ // SwTextAttr::Destroy
+ if (fn == SRCDIR "/sw/source/core/txtnode/txatbase.cxx")
+ return true;
+ // IMPL_LINK( SwDoc, AddDrawUndo, SdrUndoAction *, pUndo, void )
+ if (fn == SRCDIR "/sw/source/core/undo/undraw.cxx")
+ return true;
+ // SwHTMLParser::EndAttr
+ if (fn == SRCDIR "/sw/source/filter/html/swhtml.cxx")
+ return true;
+ // SwGlossaryHdl::Expand sometimes the pointer is owned, sometimes it is not
+ if (fn == SRCDIR "/sw/source/uibase/dochdl/gloshdl.cxx")
+ return true;
+ // SwWrtShell::Insert only owned sometimes
+ if (fn == SRCDIR "/sw/source/uibase/wrtsh/wrtsh1.cxx")
+ return true;
+ // NodeArrayDeleter
+ if (fn == SRCDIR "/unoxml/source/rdf/librdf_repository.cxx")
+ return true;
+ // SmCursor::LineToList ran out of enthusiam to rework the node handling
+ if (fn == SRCDIR "/starmath/source/cursor.cxx")
+ return true;
+ // XMLEventOASISTransformerContext::FlushEventMap
+ if (fn == SRCDIR "/xmloff/source/transform/EventOASISTContext.cxx")
+ return true;
+ // XMLEventOOoTransformerContext::FlushEventMap
+ if (fn == SRCDIR "/xmloff/source/transform/EventOOoTContext.cxx")
+ return true;
+
/*
Sometimes we can pass the param as std::unique_ptr<T>& or std::unique_ptr, sometimes the method
just needs to be inlined, which normally exposes more simplification.
*/
report(
DiagnosticsEngine::Warning,
- "calling delete on a pointer param, should be either whitelisted here or simplified",
+ "calling delete on a pointer param, should be either whitelisted or simplified",
compat::getBeginLoc(deleteExpr))
<< deleteExpr->getSourceRange();
return true;
More information about the Libreoffice-commits
mailing list