[Libreoffice-commits] core.git: compilerplugins/clang sc/source sw/inc sw/source
Noel Grandin
noel.grandin at collabora.co.uk
Thu Jul 13 09:56:39 UTC 2017
compilerplugins/clang/test/useuniqueptr.cxx | 24 +++++-
compilerplugins/clang/useuniqueptr.cxx | 106 ++++++++++++++++++++++------
sc/source/filter/excel/tokstack.cxx | 38 +++-------
sc/source/filter/inc/tokstack.hxx | 11 +-
sw/inc/bparr.hxx | 4 -
sw/source/core/bastyp/bparr.cxx | 23 ++----
sw/source/core/docnode/nodes.cxx | 2
7 files changed, 141 insertions(+), 67 deletions(-)
New commits:
commit 51a50cc95a8cb461b7026c1eb8908e17f4055076
Author: Noel Grandin <noel.grandin at collabora.co.uk>
Date: Mon Jun 26 09:48:30 2017 +0200
improve useuniqueptr loplugin to find arrays
Change-Id: I81e9d0cd4f430b11d20037054055683240792240
Reviewed-on: https://gerrit.libreoffice.org/39825
Tested-by: Jenkins <ci at libreoffice.org>
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 564e93ccbdc0..e834123264e0 100644
--- a/compilerplugins/clang/test/useuniqueptr.cxx
+++ b/compilerplugins/clang/test/useuniqueptr.cxx
@@ -8,13 +8,33 @@
*/
-class Foo {
+class Foo1 {
char* m_pbar; // expected-note {{member is here [loplugin:useuniqueptr]}}
- ~Foo()
+ ~Foo1()
{
delete m_pbar; // expected-error {{a destructor with only a single unconditional call to delete on a member, is a sure sign it should be using std::unique_ptr for that field [loplugin:useuniqueptr]}}
m_pbar = nullptr;
}
};
+
+class Foo2 {
+ char* m_pbar1; // expected-note {{member is here [loplugin:useuniqueptr]}}
+ char* m_pbar2; // expected-note {{member is here [loplugin:useuniqueptr]}}
+ ~Foo2()
+ {
+ delete[] m_pbar1; // expected-error {{managing array of trival type 'char' manually, rather use std::vector / std::array / std::unique_ptr [loplugin:useuniqueptr]}}
+ delete[] m_pbar2; // expected-error {{managing array of trival type 'char' manually, rather use std::vector / std::array / std::unique_ptr [loplugin:useuniqueptr]}}
+ }
+};
+
+class Foo3 {
+ char* m_pbar;
+ bool bMine;
+ ~Foo3()
+ {
+ if (bMine)
+ delete[] m_pbar;
+ }
+};
/* vim:set shiftwidth=4 softtabstop=4 expandtab cinoptions=b1,g0,N-s cinkeys+=0=break: */
diff --git a/compilerplugins/clang/useuniqueptr.cxx b/compilerplugins/clang/useuniqueptr.cxx
index afae4c3c770a..9e0dd33e900b 100644
--- a/compilerplugins/clang/useuniqueptr.cxx
+++ b/compilerplugins/clang/useuniqueptr.cxx
@@ -33,8 +33,11 @@ public:
TraverseDecl(compiler.getASTContext().getTranslationUnitDecl());
}
- bool VisitCXXDestructorDecl(const CXXDestructorDecl * );
- bool VisitCompoundStmt(const CompoundStmt * );
+ bool VisitCXXDestructorDecl(const CXXDestructorDecl* );
+ bool VisitCompoundStmt(const CompoundStmt* );
+private:
+ void CheckForSingleUnconditionalDelete(const CXXDestructorDecl*, const CompoundStmt* );
+ void CheckForDeleteArrayOfPOD(const CompoundStmt* );
};
bool UseUniquePtr::VisitCXXDestructorDecl(const CXXDestructorDecl* destructorDecl)
@@ -48,6 +51,14 @@ bool UseUniquePtr::VisitCXXDestructorDecl(const CXXDestructorDecl* destructorDec
if (!compoundStmt)
return true;
+ CheckForSingleUnconditionalDelete(destructorDecl, compoundStmt);
+ CheckForDeleteArrayOfPOD(compoundStmt);
+
+ return true;
+}
+
+void UseUniquePtr::CheckForSingleUnconditionalDelete(const CXXDestructorDecl* destructorDecl, const CompoundStmt* compoundStmt)
+{
const CXXDeleteExpr* deleteExpr = nullptr;
if (compoundStmt->size() == 1) {
deleteExpr = dyn_cast<CXXDeleteExpr>(compoundStmt->body_front());
@@ -66,60 +77,60 @@ bool UseUniquePtr::VisitCXXDestructorDecl(const CXXDestructorDecl* destructorDec
deleteExpr = dyn_cast<CXXDeleteExpr>(compoundStmt->body_front());
}
} else {
- return true;
+ return;
}
if (deleteExpr == nullptr)
- return true;
+ return;
const ImplicitCastExpr* pCastExpr = dyn_cast<ImplicitCastExpr>(deleteExpr->getArgument());
if (!pCastExpr)
- return true;
+ return;
const MemberExpr* pMemberExpr = dyn_cast<MemberExpr>(pCastExpr->getSubExpr());
if (!pMemberExpr)
- return true;
+ return;
// ignore union games
const FieldDecl* pFieldDecl = dyn_cast<FieldDecl>(pMemberExpr->getMemberDecl());
if (!pFieldDecl)
- return true;
+ return;
TagDecl const * td = dyn_cast<TagDecl>(pFieldDecl->getDeclContext());
if (td->isUnion())
- return true;
+ return;
// ignore calling delete on someone else's field
if (pFieldDecl->getParent() != destructorDecl->getParent() )
- return true;
+ return;
if (ignoreLocation(pFieldDecl))
- return true;
+ return;
// to ignore things like the CPPUNIT macros
StringRef aFileName = compiler.getSourceManager().getFilename(compiler.getSourceManager().getSpellingLoc(pFieldDecl->getLocStart()));
if (loplugin::hasPathnamePrefix(aFileName, WORKDIR))
- return true;
+ return;
// passes and stores pointers to member fields
if (loplugin::hasPathnamePrefix(aFileName, SRCDIR "/sot/source/sdstor/stgdir.hxx"))
- return true;
+ return;
// something platform-specific
if (loplugin::hasPathnamePrefix(aFileName, SRCDIR "/hwpfilter/source/htags.h"))
- return true;
+ return;
// @TODO there is clearly a bug in the ownership here, the operator= method cannot be right
if (loplugin::hasPathnamePrefix(aFileName, SRCDIR "/include/formula/formdata.hxx"))
- return true;
+ return;
// passes pointers to member fields
if (loplugin::hasPathnamePrefix(aFileName, SRCDIR "/sd/inc/sdpptwrp.hxx"))
- return true;
+ return;
// @TODO there is clearly a bug in the ownership here, the ScJumpMatrixToken copy constructor cannot be right
if (loplugin::hasPathnamePrefix(aFileName, SRCDIR "/sc/inc/token.hxx"))
- return true;
+ return;
// @TODO intrusive linked-lists here, with some trickiness
if (loplugin::hasPathnamePrefix(aFileName, SRCDIR "/sw/source/filter/html/parcss1.hxx"))
- return true;
+ return;
// @TODO SwDoc has some weird ref-counting going on
if (loplugin::hasPathnamePrefix(aFileName, SRCDIR "/sw/inc/shellio.hxx"))
- return true;
+ return;
// @TODO it's sharing pointers with another class
if (loplugin::hasPathnamePrefix(aFileName, SRCDIR "/sc/inc/formulacell.hxx"))
- return true;
+ return;
report(
DiagnosticsEngine::Warning,
@@ -131,7 +142,6 @@ bool UseUniquePtr::VisitCXXDestructorDecl(const CXXDestructorDecl* destructorDec
"member is here",
pFieldDecl->getLocStart())
<< pFieldDecl->getSourceRange();
- return true;
}
bool UseUniquePtr::VisitCompoundStmt(const CompoundStmt* compoundStmt)
@@ -185,6 +195,62 @@ bool UseUniquePtr::VisitCompoundStmt(const CompoundStmt* compoundStmt)
return true;
}
+void UseUniquePtr::CheckForDeleteArrayOfPOD(const CompoundStmt* compoundStmt)
+{
+ for (auto i = compoundStmt->body_begin();
+ i != compoundStmt->body_end(); ++i)
+ {
+ auto deleteExpr = dyn_cast<CXXDeleteExpr>(*i);
+ if (!deleteExpr || !deleteExpr->isArrayForm())
+ continue;
+
+ const Expr* argExpr = deleteExpr->getArgument();
+ if (auto implicitCastExpr = dyn_cast<ImplicitCastExpr>(deleteExpr->getArgument()))
+ argExpr = implicitCastExpr->getSubExpr();
+
+ auto memberExpr = dyn_cast<MemberExpr>(argExpr);
+ if (!memberExpr)
+ continue;
+ auto fieldDecl = dyn_cast<FieldDecl>(memberExpr->getMemberDecl());
+ if (!fieldDecl)
+ continue;
+ // ignore union games
+ auto * tagDecl = dyn_cast<TagDecl>(fieldDecl->getDeclContext());
+ if (tagDecl->isUnion())
+ continue;
+
+ auto pointerType = dyn_cast<PointerType>(fieldDecl->getType()->getUnqualifiedDesugaredType());
+ QualType elementType = pointerType->getPointeeType();
+ if (!elementType.isTrivialType(compiler.getASTContext()))
+ continue;
+
+ StringRef aFileName = compiler.getSourceManager().getFilename(compiler.getSourceManager().getSpellingLoc(fieldDecl->getLocStart()));
+ // TODO ignore this for now, it's just so messy to fix
+ if (loplugin::hasPathnamePrefix(aFileName, SRCDIR "/include/tools/stream.hxx"))
+ continue;
+ // TODO this is very performance sensitive code
+ if (loplugin::hasPathnamePrefix(aFileName, SRCDIR "/include/svl/itemset.hxx"))
+ continue;
+ // WW8TabBandDesc is playing games with copying/assigning itself
+ if (loplugin::hasPathnamePrefix(aFileName, SRCDIR "/sw/source/filter/ww8/ww8par.hxx"))
+ continue;
+
+ report(
+ DiagnosticsEngine::Warning,
+ "managing array of trival type %0 manually, rather use std::vector / std::array / std::unique_ptr",
+ deleteExpr->getLocStart())
+ << elementType
+ << deleteExpr->getSourceRange();
+ report(
+ DiagnosticsEngine::Note,
+ "member is here",
+ fieldDecl->getLocStart())
+ << fieldDecl->getSourceRange();
+ }
+}
+
+
+
loplugin::Plugin::Registration< UseUniquePtr > X("useuniqueptr");
}
diff --git a/sc/source/filter/excel/tokstack.cxx b/sc/source/filter/excel/tokstack.cxx
index cb5a42805b88..daf638e41fa8 100644
--- a/sc/source/filter/excel/tokstack.cxx
+++ b/sc/source/filter/excel/tokstack.cxx
@@ -48,18 +48,18 @@ TokenPool::TokenPool( svl::SharedStringPool& rSPool ) :
{
// pool for Id-sequences
nP_Id = 256;
- pP_Id = new sal_uInt16[ nP_Id ];
+ pP_Id.reset( new sal_uInt16[ nP_Id ] );
// pool for Ids
nElement = 32;
- pElement = new sal_uInt16[ nElement ];
- pType = new E_TYPE[ nElement ];
- pSize = new sal_uInt16[ nElement ];
+ pElement.reset( new sal_uInt16[ nElement ] );
+ pType.reset( new E_TYPE[ nElement ] );
+ pSize.reset( new sal_uInt16[ nElement ] );
nP_IdLast = 0;
nP_Matrix = 16;
- ppP_Matrix = new ScMatrix*[ nP_Matrix ];
- memset( ppP_Matrix, 0, sizeof( ScMatrix* ) * nP_Matrix );
+ ppP_Matrix.reset( new ScMatrix*[ nP_Matrix ] );
+ memset( ppP_Matrix.get(), 0, sizeof( ScMatrix* ) * nP_Matrix );
pScToken = new ScTokenArray;
@@ -68,13 +68,7 @@ TokenPool::TokenPool( svl::SharedStringPool& rSPool ) :
TokenPool::~TokenPool()
{
- delete[] pP_Id;
- delete[] pElement;
- delete[] pType;
- delete[] pSize;
-
ClearMatrix();
- delete[] ppP_Matrix;
delete pScToken;
}
@@ -110,8 +104,7 @@ bool TokenPool::GrowId()
nP_Id = nP_IdNew;
- delete[] pP_Id;
- pP_Id = pP_IdNew;
+ pP_Id.reset( pP_IdNew );
return true;
}
@@ -141,12 +134,9 @@ bool TokenPool::GrowElement()
nElement = nElementNew;
- delete[] pElement;
- delete[] pType;
- delete[] pSize;
- pElement = pElementNew;
- pType = pTypeNew;
- pSize = pSizeNew;
+ pElement.reset( pElementNew );
+ pType.reset( pTypeNew );
+ pSize.reset( pSizeNew );
return true;
}
@@ -161,10 +151,10 @@ bool TokenPool::GrowMatrix()
return false;
memset( ppNew, 0, sizeof( ScMatrix* ) * nNewSize );
- memcpy( ppNew, ppP_Matrix, sizeof( ScMatrix* ) * nP_Matrix );
+ for( sal_uInt16 nL = 0 ; nL < nP_Matrix ; nL++ )
+ ppNew[ nL ] = ppP_Matrix[ nL ];
- delete[] ppP_Matrix;
- ppP_Matrix = ppNew;
+ ppP_Matrix.reset( ppNew );
nP_Matrix = nNewSize;
return true;
}
@@ -202,6 +192,7 @@ bool TokenPool::GetElement( const sal_uInt16 nId )
}
break;
case T_Err:
+ break;
/* TODO: in case we had FormulaTokenArray::AddError() */
#if 0
{
@@ -212,7 +203,6 @@ bool TokenPool::GetElement( const sal_uInt16 nId )
bRet = false;
}
#endif
- break;
case T_RefC:
{
sal_uInt16 n = pElement[ nId ];
diff --git a/sc/source/filter/inc/tokstack.hxx b/sc/source/filter/inc/tokstack.hxx
index eca4d346e723..39c6561980e4 100644
--- a/sc/source/filter/inc/tokstack.hxx
+++ b/sc/source/filter/inc/tokstack.hxx
@@ -145,8 +145,7 @@ private:
TokenPoolPool<std::unique_ptr<ScSingleRefData>, 32>
ppP_RefTr; // Pool for References
-
- sal_uInt16* pP_Id; // Pool for Id-sets
+ std::unique_ptr<sal_uInt16[]> pP_Id; // Pool for Id-sets
sal_uInt16 nP_Id;
sal_uInt16 nP_IdAkt;
sal_uInt16 nP_IdLast; // last set-start
@@ -164,7 +163,7 @@ private:
TokenPoolPool<std::unique_ptr<ScSingleRefData>, 16>
ppP_Nlf;
- ScMatrix** ppP_Matrix; // Pool for Matrices
+ std::unique_ptr<ScMatrix*[]> ppP_Matrix; // Pool for Matrices
sal_uInt16 nP_Matrix;
sal_uInt16 nP_MatrixAkt;
@@ -202,9 +201,9 @@ private:
};
::std::vector<ExtAreaRef> maExtAreaRefs;
- sal_uInt16* pElement; // Array with Indices for elements
- E_TYPE* pType; // ...with Type-Info
- sal_uInt16* pSize; // ...with size (Anz. sal_uInt16)
+ std::unique_ptr<sal_uInt16[]> pElement; // Array with Indices for elements
+ std::unique_ptr<E_TYPE[]> pType; // ...with Type-Info
+ std::unique_ptr<sal_uInt16[]> pSize; // ...with size (Anz. sal_uInt16)
sal_uInt16 nElement;
sal_uInt16 nElementAkt;
diff --git a/sw/inc/bparr.hxx b/sw/inc/bparr.hxx
index 962736c49ce3..5c56f3aa81d9 100644
--- a/sw/inc/bparr.hxx
+++ b/sw/inc/bparr.hxx
@@ -25,6 +25,7 @@
#include <tools/solar.h>
#include <swdllapi.h>
#include <array>
+#include <memory>
struct BlockInfo;
class BigPtrArray;
@@ -63,7 +64,8 @@ struct BlockInfo final
class SW_DLLPUBLIC BigPtrArray
{
protected:
- BlockInfo** m_ppInf; ///< block info
+ std::unique_ptr<BlockInfo*[]>
+ m_ppInf; ///< block info
sal_uLong m_nSize; ///< number of elements
sal_uInt16 m_nMaxBlock; ///< current max. number of blocks
sal_uInt16 m_nBlock; ///< number of blocks
diff --git a/sw/source/core/bastyp/bparr.cxx b/sw/source/core/bastyp/bparr.cxx
index 446d22ef154c..7bbfdb03c35d 100644
--- a/sw/source/core/bastyp/bparr.cxx
+++ b/sw/source/core/bastyp/bparr.cxx
@@ -50,20 +50,19 @@ BigPtrArray::BigPtrArray()
m_nBlock = m_nCur = 0;
m_nSize = 0;
m_nMaxBlock = nBlockGrowSize;
- m_ppInf = new BlockInfo* [ m_nMaxBlock ];
+ m_ppInf.reset( new BlockInfo* [ m_nMaxBlock ] );
}
BigPtrArray::~BigPtrArray()
{
if( m_nBlock )
{
- BlockInfo** pp = m_ppInf;
+ BlockInfo** pp = m_ppInf.get();
for( sal_uInt16 n = 0; n < m_nBlock; ++n, ++pp )
{
delete *pp;
}
}
- delete[] m_ppInf;
}
// Also moving is done simply here. Optimization is useless because of the
@@ -138,7 +137,7 @@ sal_uInt16 BigPtrArray::Index2Block( sal_uLong pos ) const
*/
void BigPtrArray::UpdIndex( sal_uInt16 pos )
{
- BlockInfo** pp = m_ppInf + pos;
+ BlockInfo** pp = m_ppInf.get() + pos;
sal_uLong idx = (*pp)->nEnd + 1;
while( ++pos < m_nBlock )
{
@@ -161,14 +160,13 @@ BlockInfo* BigPtrArray::InsBlock( sal_uInt16 pos )
{
// than extend the array first
BlockInfo** ppNew = new BlockInfo* [ m_nMaxBlock + nBlockGrowSize ];
- memcpy( ppNew, m_ppInf, m_nMaxBlock * sizeof( BlockInfo* ));
- delete[] m_ppInf;
+ memcpy( ppNew, m_ppInf.get(), m_nMaxBlock * sizeof( BlockInfo* ));
m_nMaxBlock += nBlockGrowSize;
- m_ppInf = ppNew;
+ m_ppInf.reset( ppNew );
}
if( pos != m_nBlock )
{
- memmove( m_ppInf + pos+1, m_ppInf + pos,
+ memmove( m_ppInf.get() + pos+1, m_ppInf.get() + pos,
( m_nBlock - pos ) * sizeof( BlockInfo* ));
}
++m_nBlock;
@@ -194,9 +192,8 @@ void BigPtrArray::BlockDel( sal_uInt16 nDel )
// than shrink array
nDel = (( m_nBlock / nBlockGrowSize ) + 1 ) * nBlockGrowSize;
BlockInfo** ppNew = new BlockInfo* [ nDel ];
- memcpy( ppNew, m_ppInf, m_nBlock * sizeof( BlockInfo* ));
- delete[] m_ppInf;
- m_ppInf = ppNew;
+ memcpy( ppNew, m_ppInf.get(), m_nBlock * sizeof( BlockInfo* ));
+ m_ppInf.reset( ppNew );
m_nMaxBlock = nDel;
}
}
@@ -353,7 +350,7 @@ void BigPtrArray::Remove( sal_uLong pos, sal_uLong n )
if( ( nBlk1del + nBlkdel ) < m_nBlock )
{
- memmove( m_ppInf + nBlk1del, m_ppInf + nBlk1del + nBlkdel,
+ memmove( m_ppInf.get() + nBlk1del, m_ppInf.get() + nBlk1del + nBlkdel,
( m_nBlock - nBlkdel - nBlk1del ) * sizeof( BlockInfo* ) );
// UpdateIdx updates the successor thus start before first elem
@@ -401,7 +398,7 @@ sal_uInt16 BigPtrArray::Compress()
// Iterate over InfoBlock array from beginning to end. If there is a deleted
// block in between so move all following ones accordingly. The pointer <pp>
// represents the "old" and <qq> the "new" array.
- BlockInfo** pp = m_ppInf, **qq = pp;
+ BlockInfo** pp = m_ppInf.get(), **qq = pp;
BlockInfo* p;
BlockInfo* pLast(nullptr); // last empty block
sal_uInt16 nLast = 0; // missing elements
diff --git a/sw/source/core/docnode/nodes.cxx b/sw/source/core/docnode/nodes.cxx
index d4facd6f46bd..34d66e71d022 100644
--- a/sw/source/core/docnode/nodes.cxx
+++ b/sw/source/core/docnode/nodes.cxx
@@ -2184,7 +2184,7 @@ void SwNodes::ForEach( sal_uLong nStart, sal_uLong nEnd,
if( nStart < nEnd )
{
sal_uInt16 cur = Index2Block( nStart );
- BlockInfo** pp = m_ppInf + cur;
+ BlockInfo** pp = m_ppInf.get() + cur;
BlockInfo* p = *pp;
sal_uInt16 nElem = sal_uInt16( nStart - p->nStart );
auto pElem = p->mvData.begin() + nElem;
More information about the Libreoffice-commits
mailing list