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>

2 files changed, 108 insertions, 22 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");
 
 }