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.gran...@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 <c...@libreoffice.org> Reviewed-by: Noel Grandin <noel.gran...@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; _______________________________________________ Libreoffice-commits mailing list libreoffice-comm...@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/libreoffice-commits