teach useuniqueptr loplugin about while loops

and reduce code duplication

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

2 files changed, 125 insertions, 134 deletions

diff --git a/compilerplugins/clang/test/useuniqueptr.cxx b/compilerplugins/clang/test/useuniqueptr.cxx
index 7495a9ec192d..ef0dde3cf538 100644
--- a/compilerplugins/clang/test/useuniqueptr.cxx
+++ b/compilerplugins/clang/test/useuniqueptr.cxx
@@ -99,6 +99,7 @@ class Foo9 {
     XXX* m_pbar1; // expected-note {{member is here [loplugin:useuniqueptr]}}
     XXX* m_pbar2; // expected-note {{member is here [loplugin:useuniqueptr]}}
     XXX* m_pbar3; // expected-note {{member is here [loplugin:useuniqueptr]}}
+    XXX* m_pbar4; // expected-note {{member is here [loplugin:useuniqueptr]}}
     ~Foo9()
     {
         if (m_pbar1)
@@ -111,6 +112,12 @@ class Foo9 {
         }
         if (m_pbar3 != nullptr)
             delete m_pbar3; // expected-error {{unconditional call to delete on a member, should be using std::unique_ptr [loplugin:useuniqueptr]}}
+        if (m_pbar4 != nullptr)
+        {
+            int x = 1;
+            (void)x;
+            delete m_pbar4; // expected-error {{unconditional call to delete on a member, should be using std::unique_ptr [loplugin:useuniqueptr]}}
+        }
     }
 };
 // no warning expected
@@ -135,4 +142,13 @@ class Foo11 {
         }
     }
 };
+class Foo12 {
+    std::array<int*,10> m_pbar; // expected-note {{member is here [loplugin:useuniqueptr]}}
+    ~Foo12()
+    {
+        int i = 0;
+        while (i < 10)
+            delete m_pbar[i++]; // expected-error {{rather manage with std::some_container<std::unique_ptr<T>> [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 56b669a5e1e1..d2e28860592a 100644
--- a/compilerplugins/clang/useuniqueptr.cxx
+++ b/compilerplugins/clang/useuniqueptr.cxx
@@ -39,9 +39,13 @@ public:
     bool VisitCompoundStmt(const CompoundStmt* );
 private:
     void CheckForUnconditionalDelete(const CXXDestructorDecl*, const CompoundStmt* );
-    void CheckDeleteExpr(const CXXDestructorDecl*, const CXXDeleteExpr* );
-    void CheckForForLoopDelete(const CXXDestructorDecl*, const CompoundStmt* );
-    void CheckForRangedLoopDelete(const CXXDestructorDecl*, const CompoundStmt* );
+    void CheckForSimpleDelete(const CXXDestructorDecl*, const CompoundStmt* );
+    void CheckRangedLoopDelete(const CXXDestructorDecl*, const CXXForRangeStmt* );
+    void CheckLoopDelete(const CXXDestructorDecl*, const Stmt* );
+    void CheckLoopDelete(const CXXDestructorDecl*, const CXXDeleteExpr* );
+    void CheckDeleteExpr(const CXXDestructorDecl*, const CXXDeleteExpr*);
+    void CheckDeleteExpr(const CXXDestructorDecl*, const CXXDeleteExpr*,
+        const MemberExpr*, StringRef message);
 };
 
 bool UseUniquePtr::VisitCXXDestructorDecl(const CXXDestructorDecl* destructorDecl)
@@ -55,14 +59,25 @@ bool UseUniquePtr::VisitCXXDestructorDecl(const CXXDestructorDecl* destructorDec
     if (!compoundStmt || compoundStmt->size() == 0)
         return true;
 
-    CheckForUnconditionalDelete(destructorDecl, compoundStmt);
-    CheckForForLoopDelete(destructorDecl, compoundStmt);
-    CheckForRangedLoopDelete(destructorDecl, compoundStmt);
+    CheckForSimpleDelete(destructorDecl, compoundStmt);
+
+    for (auto i = compoundStmt->body_begin(); i != compoundStmt->body_end(); ++i)
+    {
+        if (auto cxxForRangeStmt = dyn_cast<CXXForRangeStmt>(*i))
+            CheckRangedLoopDelete(destructorDecl, cxxForRangeStmt);
+        else if (auto forStmt = dyn_cast<ForStmt>(*i))
+            CheckLoopDelete(destructorDecl, forStmt->getBody());
+        else if (auto whileStmt = dyn_cast<WhileStmt>(*i))
+            CheckLoopDelete(destructorDecl, whileStmt->getBody());
+    }
 
     return true;
 }
 
-void UseUniquePtr::CheckForUnconditionalDelete(const CXXDestructorDecl* destructorDecl, const CompoundStmt* compoundStmt)
+/**
+ * check for simple call to delete in a destructor i.e. direct unconditional call, or if-guarded call
+ */
+void UseUniquePtr::CheckForSimpleDelete(const CXXDestructorDecl* destructorDecl, const CompoundStmt* compoundStmt)
 {
     for (auto i = compoundStmt->body_begin(); i != compoundStmt->body_end(); ++i)
     {
@@ -83,6 +98,7 @@ void UseUniquePtr::CheckForUnconditionalDelete(const CXXDestructorDecl* destruct
             // ignore "if (bMine)"
             if (!loplugin::TypeCheck(ifCondMemberExpr->getType()).Pointer())
                 continue;
+            // good
         }
         else if (auto binaryOp = dyn_cast<BinaryOperator>(cond))
         {
@@ -90,15 +106,18 @@ void UseUniquePtr::CheckForUnconditionalDelete(const CXXDestructorDecl* destruct
                 continue;
             if (!isa<CXXNullPtrLiteralExpr>(binaryOp->getRHS()->IgnoreImpCasts()))
                 continue;
+            // good
         }
-        else
+        else // ignore anything more complicated
             continue;
+
         deleteExpr = dyn_cast<CXXDeleteExpr>(ifStmt->getThen());
         if (deleteExpr)
         {
             CheckDeleteExpr(destructorDecl, deleteExpr);
             continue;
         }
+
         auto ifThenCompoundStmt = dyn_cast<CompoundStmt>(ifStmt->getThen());
         if (!ifThenCompoundStmt)
             continue;
@@ -116,29 +135,38 @@ void UseUniquePtr::CheckForUnconditionalDelete(const CXXDestructorDecl* destruct
  */
 void UseUniquePtr::CheckDeleteExpr(const CXXDestructorDecl* destructorDecl, const CXXDeleteExpr* deleteExpr)
 {
-    const ImplicitCastExpr* pCastExpr = dyn_cast<ImplicitCastExpr>(deleteExpr->getArgument());
-    if (!pCastExpr)
+    const ImplicitCastExpr* castExpr = dyn_cast<ImplicitCastExpr>(deleteExpr->getArgument());
+    if (!castExpr)
         return;
-    const MemberExpr* pMemberExpr = dyn_cast<MemberExpr>(pCastExpr->getSubExpr());
-    if (!pMemberExpr)
+    const MemberExpr* memberExpr = dyn_cast<MemberExpr>(castExpr->getSubExpr());
+    if (!memberExpr)
         return;
+    CheckDeleteExpr(destructorDecl, deleteExpr, memberExpr,
+        "unconditional call to delete on a member, should be using std::unique_ptr");
+}
 
+/**
+ * Check the delete expression in a destructor.
+ */
+void UseUniquePtr::CheckDeleteExpr(const CXXDestructorDecl* destructorDecl, const CXXDeleteExpr* deleteExpr,
+    const MemberExpr* memberExpr, StringRef message)
+{
     // ignore union games
-    const FieldDecl* pFieldDecl = dyn_cast<FieldDecl>(pMemberExpr->getMemberDecl());
-    if (!pFieldDecl)
+    const FieldDecl* fieldDecl = dyn_cast<FieldDecl>(memberExpr->getMemberDecl());
+    if (!fieldDecl)
         return;
-    TagDecl const * td = dyn_cast<TagDecl>(pFieldDecl->getDeclContext());
+    TagDecl const * td = dyn_cast<TagDecl>(fieldDecl->getDeclContext());
     if (td->isUnion())
         return;
 
     // ignore calling delete on someone else's field
-    if (pFieldDecl->getParent() != destructorDecl->getParent() )
+    if (fieldDecl->getParent() != destructorDecl->getParent() )
         return;
 
-    if (ignoreLocation(pFieldDecl))
+    if (ignoreLocation(fieldDecl))
         return;
     // to ignore things like the CPPUNIT macros
-    StringRef aFileName = compiler.getSourceManager().getFilename(compiler.getSourceManager().getSpellingLoc(pFieldDecl->getLocStart()));
+    StringRef aFileName = compiler.getSourceManager().getFilename(compiler.getSourceManager().getSpellingLoc(fieldDecl->getLocStart()));
     if (loplugin::hasPathnamePrefix(aFileName, WORKDIR))
         return;
     // passes and stores pointers to member fields
@@ -169,145 +197,92 @@ void UseUniquePtr::CheckDeleteExpr(const CXXDestructorDecl* destructorDecl, cons
         return;
     if (loplugin::hasPathnamePrefix(aFileName, SRCDIR "/include/sot/"))
         return;
+    // the std::vector is being passed to another class
+    if (loplugin::hasPathnamePrefix(aFileName, SRCDIR "/sfx2/source/explorer/nochaos.cxx"))
+        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;
+    // there is a loop in ~ImplPrnQueueList deleting stuff on a global data structure
+    if (loplugin::hasPathnamePrefix(aFileName, SRCDIR "/vcl/inc/print.h"))
+        return;
+    // painful linked list
+    if (loplugin::hasPathnamePrefix(aFileName, SRCDIR "/basic/source/inc/runtime.hxx"))
+        return;
 
     report(
         DiagnosticsEngine::Warning,
-        "unconditional call to delete on a member, should be using std::unique_ptr",
+        message,
         deleteExpr->getLocStart())
         << deleteExpr->getSourceRange();
     report(
         DiagnosticsEngine::Note,
         "member is here",
-        pFieldDecl->getLocStart())
-        << pFieldDecl->getSourceRange();
+        fieldDecl->getLocStart())
+        << fieldDecl->getSourceRange();
 }
 
-void UseUniquePtr::CheckForForLoopDelete(const CXXDestructorDecl* destructorDecl, const CompoundStmt* compoundStmt)
+void UseUniquePtr::CheckLoopDelete(const CXXDestructorDecl* destructorDecl, const Stmt* bodyStmt)
 {
-    for (auto i = compoundStmt->body_begin(); i != compoundStmt->body_end(); ++i)
+    if (auto deleteExpr = dyn_cast<CXXDeleteExpr>(bodyStmt))
+        CheckLoopDelete(destructorDecl, deleteExpr);
+    else if (auto compoundStmt = dyn_cast<CompoundStmt>(bodyStmt))
     {
-        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 (;;)
+        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))
-            {
-                if (cxxOperatorCallExpr->getOperator() == OO_Subscript)
-                {
-                    memberExpr = dyn_cast<MemberExpr>(cxxOperatorCallExpr->getArg(0));
-                    break;
-                }
-                break;
-            }
-            else
-                break;
+            if (auto deleteExpr = dyn_cast<CXXDeleteExpr>(*i))
+                CheckLoopDelete(destructorDecl, deleteExpr);
         }
-        if (!memberExpr)
-            continue;
-
-        // 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;
-
-        // 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;
-        // the std::vector is being passed to another class
-        if (loplugin::hasPathnamePrefix(aFileName, SRCDIR "/sfx2/source/explorer/nochaos.cxx"))
-            return;
-
-        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)
+void UseUniquePtr::CheckLoopDelete(const CXXDestructorDecl* destructorDecl, const CXXDeleteExpr* deleteExpr)
 {
-    for (auto i = compoundStmt->body_begin(); i != compoundStmt->body_end(); ++i)
+    const MemberExpr* memberExpr = nullptr;
+    const Expr* subExpr = deleteExpr->getArgument();
+    for (;;)
     {
-        auto cxxForRangeStmt = dyn_cast<CXXForRangeStmt>(*i);
-        if (!cxxForRangeStmt)
-            continue;
-        CXXDeleteExpr const * deleteExpr = nullptr;
-        if (auto compoundStmt = dyn_cast<CompoundStmt>(cxxForRangeStmt->getBody()))
-            deleteExpr = dyn_cast<CXXDeleteExpr>(*compoundStmt->body_begin());
+        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))
+        {
+            if (cxxOperatorCallExpr->getOperator() == OO_Subscript)
+            {
+                memberExpr = dyn_cast<MemberExpr>(cxxOperatorCallExpr->getArg(0));
+                break;
+            }
+            break;
+        }
         else
-            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;
-
-        // 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;
+           break;
+    }
+    if (!memberExpr)
+        return;
 
-        if (ignoreLocation(fieldDecl))
-            continue;
+    CheckDeleteExpr(destructorDecl, deleteExpr, memberExpr, "rather manage with std::some_container<std::unique_ptr<T>>");
+}
 
-        // 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;
-        // there is a loop in ~ImplPrnQueueList deleting stuff on a global data structure
-        if (loplugin::hasPathnamePrefix(aFileName, SRCDIR "/vcl/inc/print.h"))
-            return;
+void UseUniquePtr::CheckRangedLoopDelete(const CXXDestructorDecl* destructorDecl, const CXXForRangeStmt* cxxForRangeStmt)
+{
+    CXXDeleteExpr const * deleteExpr = nullptr;
+    if (auto compoundStmt = dyn_cast<CompoundStmt>(cxxForRangeStmt->getBody()))
+        deleteExpr = dyn_cast<CXXDeleteExpr>(*compoundStmt->body_begin());
+    else
+        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;
 
-        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();
-    }
+    CheckDeleteExpr(destructorDecl, deleteExpr, memberExpr, "rather manage with std::some_container<std::unique_ptr<T>>");
 }
 
 bool UseUniquePtr::VisitCompoundStmt(const CompoundStmt* compoundStmt)