improve useuniqueptr loplugin to find arrays

Change-Id: I81e9d0cd4f430b11d20037054055683240792240
Reviewed-on: https://gerrit.libreoffice.org/39825
Tested-by: Jenkins <ci@libreoffice.org>
Reviewed-by: Noel Grandin <noel.grandin@collabora.co.uk>

7 files changed, 141 insertions, 67 deletions

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;