loplugin:useuniqueptr in l10ntools

update plugin to find all places where we are unconditionally deleting
stuff in a destructor

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

2 files changed, 179 insertions, 248 deletions

diff --git a/compilerplugins/clang/test/useuniqueptr.cxx b/compilerplugins/clang/test/useuniqueptr.cxx
index b45781925630..2ffdecc33a04 100644
--- a/compilerplugins/clang/test/useuniqueptr.cxx
+++ b/compilerplugins/clang/test/useuniqueptr.cxx
@@ -18,7 +18,7 @@ class Foo1 {
     XXX* m_pbar; // expected-note {{member is here [loplugin:useuniqueptr]}}
     ~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]}}
+        delete m_pbar; // expected-error {{unconditional call to delete on a member, should be using std::unique_ptr [loplugin:useuniqueptr]}}
         m_pbar = nullptr;
     }
 };
@@ -29,8 +29,8 @@ class Foo2 {
     char* m_pbar2; // expected-note {{member is here [loplugin:useuniqueptr]}}
     ~Foo2()
     {
-        delete[] m_pbar1; // expected-error {{managing POD type 'char' manually, rather use std::vector / std::array / std::unique_ptr [loplugin:useuniqueptr]}}
-        delete[] m_pbar2; // expected-error {{managing POD type 'char' manually, rather use std::vector / std::array / std::unique_ptr [loplugin:useuniqueptr]}}
+        delete[] m_pbar1; // expected-error {{unconditional call to delete on a member, should be using std::unique_ptr [loplugin:useuniqueptr]}}
+        delete[] m_pbar2; // expected-error {{unconditional call to delete on a member, should be using std::unique_ptr [loplugin:useuniqueptr]}}
     }
 };
 
@@ -85,4 +85,13 @@ class Class8 {
             delete i.second;
     }
 };
+class Foo8 {
+    XXX* m_pbar1; // expected-note {{member is here [loplugin:useuniqueptr]}}
+    XXX* m_pbar2; // expected-note {{member is here [loplugin:useuniqueptr]}}
+    ~Foo8()
+    {
+        delete m_pbar1; // expected-error {{unconditional call to delete on a member, should be using std::unique_ptr [loplugin:useuniqueptr]}}
+        delete m_pbar2; // expected-error {{unconditional call to delete on a member, should be using std::unique_ptr [loplugin:useuniqueptr]}}
+    }
+};
 /* 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 c35426727cdc..0c0e25d2ebee 100644
--- a/compilerplugins/clang/useuniqueptr.cxx
+++ b/compilerplugins/clang/useuniqueptr.cxx
@@ -38,10 +38,9 @@ public:
     bool VisitCXXDestructorDecl(const CXXDestructorDecl* );
     bool VisitCompoundStmt(const CompoundStmt* );
 private:
-    void CheckForSingleUnconditionalDelete(const CXXDestructorDecl*, const CompoundStmt* );
+    void CheckForUnconditionalDelete(const CXXDestructorDecl*, const CompoundStmt* );
     void CheckForForLoopDelete(const CXXDestructorDecl*, const CompoundStmt* );
     void CheckForRangedLoopDelete(const CXXDestructorDecl*, const CompoundStmt* );
-    void CheckForDeleteOfPOD(const CompoundStmt* );
 };
 
 bool UseUniquePtr::VisitCXXDestructorDecl(const CXXDestructorDecl* destructorDecl)
@@ -52,215 +51,197 @@ bool UseUniquePtr::VisitCXXDestructorDecl(const CXXDestructorDecl* destructorDec
         return true;
 
     const CompoundStmt* compoundStmt = dyn_cast_or_null< CompoundStmt >( destructorDecl->getBody() );
-    if (!compoundStmt)
+    if (!compoundStmt || compoundStmt->size() == 0)
         return true;
 
-    if (compoundStmt->size() > 0)
-    {
-        CheckForSingleUnconditionalDelete(destructorDecl, compoundStmt);
-        CheckForDeleteOfPOD(compoundStmt);
-        CheckForForLoopDelete(destructorDecl, compoundStmt);
-        CheckForRangedLoopDelete(destructorDecl, compoundStmt);
-    }
+    CheckForUnconditionalDelete(destructorDecl, compoundStmt);
+    CheckForForLoopDelete(destructorDecl, compoundStmt);
+    CheckForRangedLoopDelete(destructorDecl, compoundStmt);
 
     return true;
 }
 
-void UseUniquePtr::CheckForSingleUnconditionalDelete(const CXXDestructorDecl* destructorDecl, const CompoundStmt* compoundStmt)
+void UseUniquePtr::CheckForUnconditionalDelete(const CXXDestructorDecl* destructorDecl, const CompoundStmt* compoundStmt)
 {
-    const CXXDeleteExpr* deleteExpr = nullptr;
-    if (compoundStmt->size() == 1) {
-        deleteExpr = dyn_cast<CXXDeleteExpr>(compoundStmt->body_front());
-    }
-    else if (compoundStmt->size() == 2) {
-        // ignore SAL_INFO type stuff
-        // @TODO should probably be a little more specific here
-        if (isa<DoStmt>(compoundStmt->body_front())) {
-            deleteExpr = dyn_cast<CXXDeleteExpr>(compoundStmt->body_back());
-        }
-        // look for the following pattern:
-        // delete m_pbar;
-        // m_pbar = nullptr;
-        else if (auto binaryOp = dyn_cast<BinaryOperator>(compoundStmt->body_back())) {
-            if (binaryOp->getOpcode() == BO_Assign)
-                deleteExpr = dyn_cast<CXXDeleteExpr>(compoundStmt->body_front());
-        }
-    } else {
-        // look for the following pattern:
-        // delete m_pbar;
-        // m_pbar = nullptr;
-        if (auto binaryOp = dyn_cast<BinaryOperator>(compoundStmt->body_back())) {
-            if (binaryOp->getOpcode() == BO_Assign)
-                deleteExpr = dyn_cast<CXXDeleteExpr>(*(++compoundStmt->body_rbegin()));
-        }
-    }
-    if (!deleteExpr)
-        return;
+    for (auto i = compoundStmt->body_begin(); i != compoundStmt->body_end(); ++i)
+    {
+        auto deleteExpr = dyn_cast<CXXDeleteExpr>(*i);
+        if (!deleteExpr)
+            continue;
 
-    const ImplicitCastExpr* pCastExpr = dyn_cast<ImplicitCastExpr>(deleteExpr->getArgument());
-    if (!pCastExpr)
-        return;
-    const MemberExpr* pMemberExpr = dyn_cast<MemberExpr>(pCastExpr->getSubExpr());
-    if (!pMemberExpr)
-        return;
-
-    // ignore union games
-    const FieldDecl* pFieldDecl = dyn_cast<FieldDecl>(pMemberExpr->getMemberDecl());
-    if (!pFieldDecl)
-        return;
-    TagDecl const * td = dyn_cast<TagDecl>(pFieldDecl->getDeclContext());
-    if (td->isUnion())
-        return;
-
-    // ignore calling delete on someone else's field
-    if (pFieldDecl->getParent() != destructorDecl->getParent() )
-        return;
-
-    if (ignoreLocation(pFieldDecl))
-        return;
-    // to ignore things like the CPPUNIT macros
-    StringRef aFileName = compiler.getSourceManager().getFilename(compiler.getSourceManager().getSpellingLoc(pFieldDecl->getLocStart()));
-    if (loplugin::hasPathnamePrefix(aFileName, WORKDIR))
-        return;
-    // passes and stores pointers to member fields
-    if (loplugin::hasPathnamePrefix(aFileName, SRCDIR "/sot/source/sdstor/stgdir.hxx"))
-        return;
-    // something platform-specific
-    if (loplugin::hasPathnamePrefix(aFileName, SRCDIR "/hwpfilter/source/htags.h"))
-        return;
-    // passes pointers to member fields
-    if (loplugin::hasPathnamePrefix(aFileName, SRCDIR "/sd/inc/sdpptwrp.hxx"))
-        return;
-    // @TODO intrusive linked-lists here, with some trickiness
-    if (loplugin::hasPathnamePrefix(aFileName, SRCDIR "/sw/source/filter/html/parcss1.hxx"))
-        return;
-    // @TODO SwDoc has some weird ref-counting going on
-    if (loplugin::hasPathnamePrefix(aFileName, SRCDIR "/sw/inc/shellio.hxx"))
-        return;
-    // @TODO it's sharing pointers with another class
-    if (loplugin::hasPathnamePrefix(aFileName, SRCDIR "/sc/inc/formulacell.hxx"))
-        return;
+        const ImplicitCastExpr* pCastExpr = dyn_cast<ImplicitCastExpr>(deleteExpr->getArgument());
+        if (!pCastExpr)
+            continue;
+        const MemberExpr* pMemberExpr = dyn_cast<MemberExpr>(pCastExpr->getSubExpr());
+        if (!pMemberExpr)
+            continue;
 
-    report(
-        DiagnosticsEngine::Warning,
-        "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",
-        deleteExpr->getLocStart())
-        << deleteExpr->getSourceRange();
-    report(
-        DiagnosticsEngine::Note,
-        "member is here",
-        pFieldDecl->getLocStart())
-        << pFieldDecl->getSourceRange();
+        // ignore union games
+        const FieldDecl* pFieldDecl = dyn_cast<FieldDecl>(pMemberExpr->getMemberDecl());
+        if (!pFieldDecl)
+            continue;
+        TagDecl const * td = dyn_cast<TagDecl>(pFieldDecl->getDeclContext());
+        if (td->isUnion())
+            continue;
+
+        // ignore calling delete on someone else's field
+        if (pFieldDecl->getParent() != destructorDecl->getParent() )
+            continue;
+
+        if (ignoreLocation(pFieldDecl))
+            continue;
+        // to ignore things like the CPPUNIT macros
+        StringRef aFileName = compiler.getSourceManager().getFilename(compiler.getSourceManager().getSpellingLoc(pFieldDecl->getLocStart()));
+        if (loplugin::hasPathnamePrefix(aFileName, WORKDIR))
+            continue;
+        // passes and stores pointers to member fields
+        if (loplugin::hasPathnamePrefix(aFileName, SRCDIR "/sot/source/sdstor/stgdir.hxx"))
+            continue;
+        // something platform-specific
+        if (loplugin::hasPathnamePrefix(aFileName, SRCDIR "/hwpfilter/source/htags.h"))
+            continue;
+        // passes pointers to member fields
+        if (loplugin::hasPathnamePrefix(aFileName, SRCDIR "/sd/inc/sdpptwrp.hxx"))
+            continue;
+        // @TODO intrusive linked-lists here, with some trickiness
+        if (loplugin::hasPathnamePrefix(aFileName, SRCDIR "/sw/source/filter/html/parcss1.hxx"))
+            continue;
+        // @TODO SwDoc has some weird ref-counting going on
+        if (loplugin::hasPathnamePrefix(aFileName, SRCDIR "/sw/inc/shellio.hxx"))
+            continue;
+        // @TODO it's sharing pointers with another class
+        if (loplugin::hasPathnamePrefix(aFileName, SRCDIR "/sc/inc/formulacell.hxx"))
+            continue;
+
+        report(
+            DiagnosticsEngine::Warning,
+            "unconditional call to delete on a member, should be using std::unique_ptr",
+            deleteExpr->getLocStart())
+            << deleteExpr->getSourceRange();
+        report(
+            DiagnosticsEngine::Note,
+            "member is here",
+            pFieldDecl->getLocStart())
+            << pFieldDecl->getSourceRange();
+    }
 }
 
 void UseUniquePtr::CheckForForLoopDelete(const CXXDestructorDecl* destructorDecl, const CompoundStmt* compoundStmt)
 {
-    auto forStmt = dyn_cast<ForStmt>(compoundStmt->body_back());
-    if (!forStmt)
-        return;
-    auto deleteExpr = dyn_cast<CXXDeleteExpr>(forStmt->getBody());
-    if (!deleteExpr)
-        return;
-
-    const MemberExpr* memberExpr = nullptr;
-    const Expr* subExpr = deleteExpr->getArgument();
-    for (;;)
+    for (auto i = compoundStmt->body_begin(); i != compoundStmt->body_end(); ++i)
     {
-        subExpr = subExpr->IgnoreImpCasts();
-        if ((memberExpr = dyn_cast<MemberExpr>(subExpr)))
-            break;
-        else if (auto arraySubscriptExpr = dyn_cast<ArraySubscriptExpr>(subExpr))
-            subExpr = arraySubscriptExpr->getBase();
-        else if (auto cxxOperatorCallExpr = dyn_cast<CXXOperatorCallExpr>(subExpr))
+        auto forStmt = dyn_cast<ForStmt>(*i);
+        if (!forStmt)
+            continue;
+        auto deleteExpr = dyn_cast<CXXDeleteExpr>(forStmt->getBody());
+        if (!deleteExpr)
+            continue;
+
+        const MemberExpr* memberExpr = nullptr;
+        const Expr* subExpr = deleteExpr->getArgument();
+        for (;;)
         {
-            if (cxxOperatorCallExpr->getOperator() == OO_Subscript)
+            subExpr = subExpr->IgnoreImpCasts();
+            if ((memberExpr = dyn_cast<MemberExpr>(subExpr)))
+                break;
+            else if (auto arraySubscriptExpr = dyn_cast<ArraySubscriptExpr>(subExpr))
+                subExpr = arraySubscriptExpr->getBase();
+            else if (auto cxxOperatorCallExpr = dyn_cast<CXXOperatorCallExpr>(subExpr))
             {
-                memberExpr = dyn_cast<MemberExpr>(cxxOperatorCallExpr->getArg(0));
+                if (cxxOperatorCallExpr->getOperator() == OO_Subscript)
+                {
+                    memberExpr = dyn_cast<MemberExpr>(cxxOperatorCallExpr->getArg(0));
+                    break;
+                }
                 break;
             }
-            return;
+            else
+                break;
         }
-        else
-            return;
-    }
+        if (!memberExpr)
+            continue;
 
-    // ignore union games
-    const FieldDecl* fieldDecl = dyn_cast<FieldDecl>(memberExpr->getMemberDecl());
-    if (!fieldDecl)
-        return;
-    TagDecl const * td = dyn_cast<TagDecl>(fieldDecl->getDeclContext());
-    if (td->isUnion())
-        return;
-
-    // ignore calling delete on someone else's field
-    if (fieldDecl->getParent() != destructorDecl->getParent() )
-        return;
-
-    if (ignoreLocation(fieldDecl))
-        return;
-    // to ignore things like the CPPUNIT macros
-    StringRef aFileName = compiler.getSourceManager().getFilename(compiler.getSourceManager().getSpellingLoc(fieldDecl->getLocStart()));
-    if (loplugin::hasPathnamePrefix(aFileName, WORKDIR))
-        return;
+        // ignore union games
+        const FieldDecl* fieldDecl = dyn_cast<FieldDecl>(memberExpr->getMemberDecl());
+        if (!fieldDecl)
+            continue;
+        TagDecl const * td = dyn_cast<TagDecl>(fieldDecl->getDeclContext());
+        if (td->isUnion())
+            continue;
 
-    report(
-        DiagnosticsEngine::Warning,
-        "rather manage with std::some_container<std::unique_ptr<T>>",
-        deleteExpr->getLocStart())
-        << deleteExpr->getSourceRange();
-    report(
-        DiagnosticsEngine::Note,
-        "member is here",
-        fieldDecl->getLocStart())
-        << fieldDecl->getSourceRange();
+        // ignore calling delete on someone else's field
+        if (fieldDecl->getParent() != destructorDecl->getParent() )
+            continue;
+
+        if (ignoreLocation(fieldDecl))
+            continue;
+        // to ignore things like the CPPUNIT macros
+        StringRef aFileName = compiler.getSourceManager().getFilename(compiler.getSourceManager().getSpellingLoc(fieldDecl->getLocStart()));
+        if (loplugin::hasPathnamePrefix(aFileName, WORKDIR))
+            continue;
+
+        report(
+            DiagnosticsEngine::Warning,
+            "rather manage with std::some_container<std::unique_ptr<T>>",
+            deleteExpr->getLocStart())
+            << deleteExpr->getSourceRange();
+        report(
+            DiagnosticsEngine::Note,
+            "member is here",
+            fieldDecl->getLocStart())
+            << fieldDecl->getSourceRange();
+    }
 }
 
 void UseUniquePtr::CheckForRangedLoopDelete(const CXXDestructorDecl* destructorDecl, const CompoundStmt* compoundStmt)
 {
-    auto cxxForRangeStmt = dyn_cast<CXXForRangeStmt>(compoundStmt->body_back());
-    if (!cxxForRangeStmt)
-        return;
-    auto deleteExpr = dyn_cast<CXXDeleteExpr>(cxxForRangeStmt->getBody());
-    if (!deleteExpr)
-        return;
-    auto memberExpr = dyn_cast<MemberExpr>(cxxForRangeStmt->getRangeInit());
-    if (!memberExpr)
-        return;
-    auto fieldDecl = dyn_cast<FieldDecl>(memberExpr->getMemberDecl());
-    if (!fieldDecl)
-        return;
-
-    // ignore union games
-    TagDecl const * td = dyn_cast<TagDecl>(fieldDecl->getDeclContext());
-    if (td->isUnion())
-        return;
-
-    // ignore calling delete on someone else's field
-    if (fieldDecl->getParent() != destructorDecl->getParent() )
-        return;
-
-    if (ignoreLocation(fieldDecl))
-        return;
-
-    // to ignore things like the CPPUNIT macros
-    StringRef aFileName = compiler.getSourceManager().getFilename(compiler.getSourceManager().getSpellingLoc(fieldDecl->getLocStart()));
-    if (loplugin::hasPathnamePrefix(aFileName, WORKDIR))
-        return;
-    // ignore std::map and std::unordered_map, MSVC 2015 has problems with mixing these with std::unique_ptr
-    auto tc = loplugin::TypeCheck(fieldDecl->getType());
-    if (tc.Class("map").StdNamespace() || tc.Class("unordered_map").StdNamespace())
-        return;
+    for (auto i = compoundStmt->body_begin(); i != compoundStmt->body_end(); ++i)
+    {
+        auto cxxForRangeStmt = dyn_cast<CXXForRangeStmt>(*i);
+        if (!cxxForRangeStmt)
+            continue;
+        auto deleteExpr = dyn_cast<CXXDeleteExpr>(cxxForRangeStmt->getBody());
+        if (!deleteExpr)
+            continue;
+        auto memberExpr = dyn_cast<MemberExpr>(cxxForRangeStmt->getRangeInit());
+        if (!memberExpr)
+            continue;
+        auto fieldDecl = dyn_cast<FieldDecl>(memberExpr->getMemberDecl());
+        if (!fieldDecl)
+            continue;
 
-    report(
-        DiagnosticsEngine::Warning,
-        "rather manage with std::some_container<std::unique_ptr<T>>",
-        deleteExpr->getLocStart())
-        << deleteExpr->getSourceRange();
-    report(
-        DiagnosticsEngine::Note,
-        "member is here",
-        fieldDecl->getLocStart())
-        << fieldDecl->getSourceRange();
+        // ignore union games
+        TagDecl const * td = dyn_cast<TagDecl>(fieldDecl->getDeclContext());
+        if (td->isUnion())
+            continue;
+
+        // ignore calling delete on someone else's field
+        if (fieldDecl->getParent() != destructorDecl->getParent() )
+            continue;
+
+        if (ignoreLocation(fieldDecl))
+            continue;
+
+        // to ignore things like the CPPUNIT macros
+        StringRef aFileName = compiler.getSourceManager().getFilename(compiler.getSourceManager().getSpellingLoc(fieldDecl->getLocStart()));
+        if (loplugin::hasPathnamePrefix(aFileName, WORKDIR))
+            continue;
+        // ignore std::map and std::unordered_map, MSVC 2015 has problems with mixing these with std::unique_ptr
+        auto tc = loplugin::TypeCheck(fieldDecl->getType());
+        if (tc.Class("map").StdNamespace() || tc.Class("unordered_map").StdNamespace())
+            continue;
+
+        report(
+            DiagnosticsEngine::Warning,
+            "rather manage with std::some_container<std::unique_ptr<T>>",
+            deleteExpr->getLocStart())
+            << deleteExpr->getSourceRange();
+        report(
+            DiagnosticsEngine::Note,
+            "member is here",
+            fieldDecl->getLocStart())
+            << fieldDecl->getSourceRange();
+    }
 }
 
 bool UseUniquePtr::VisitCompoundStmt(const CompoundStmt* compoundStmt)
@@ -314,66 +295,7 @@ bool UseUniquePtr::VisitCompoundStmt(const CompoundStmt* compoundStmt)
     return true;
 }
 
-void UseUniquePtr::CheckForDeleteOfPOD(const CompoundStmt* compoundStmt)
-{
-    for (auto i = compoundStmt->body_begin();
-              i != compoundStmt->body_end(); ++i)
-    {
-        auto deleteExpr = dyn_cast<CXXDeleteExpr>(*i);
-        if (!deleteExpr)
-            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<clang::PointerType>(fieldDecl->getType()->getUnqualifiedDesugaredType());
-        QualType elementType = pointerType->getPointeeType();
-        auto tc = loplugin::TypeCheck(elementType);
-        if (!elementType.isPODType(compiler.getASTContext())
-            && !tc.Class("OUString").Namespace("rtl").GlobalNamespace()
-            && !tc.Class("OString").Namespace("rtl").GlobalNamespace())
-            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 POD 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", true);
+loplugin::Plugin::Registration< UseUniquePtr > X("useuniqueptr", false);
 
 }