loplugin:useuniqueptr, look for containers..

that can use std::unique_ptr, and apply it in i18npool

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

compilerplugins/clang/test/useuniqueptr.cxx

@@ -7,6 +7,9 @@
  * file, You can obtain one at http://mozilla.org/MPL/2.0/.
  */
 
+#include <array>
+#include <unordered_map>
+
 struct XXX {
     ~XXX() {}
 };
@@ -40,4 +43,46 @@ class Foo3 {
             delete[] m_pbar;
     }
 };
+
+class Class4 {
+    int* m_pbar[10]; // expected-note {{member is here [loplugin:useuniqueptr]}}
+    ~Class4()
+    {
+        for (int i = 0; i < 10; ++i)
+            delete m_pbar[i]; // expected-error {{rather manage with std::some_container<std::unique_ptr<T>> [loplugin:useuniqueptr]}}
+    }
+};
+class Class5 {
+    int* m_pbar[10]; // expected-note {{member is here [loplugin:useuniqueptr]}}
+    ~Class5()
+    {
+        for (auto p : m_pbar)
+            delete p; // expected-error {{rather manage with std::some_container<std::unique_ptr<T>> [loplugin:useuniqueptr]}}
+    }
+};
+class Class6 {
+    std::array<int*,10> m_pbar; // expected-note {{member is here [loplugin:useuniqueptr]}}
+    ~Class6()
+    {
+        for (auto p : m_pbar)
+            delete p; // expected-error {{rather manage with std::some_container<std::unique_ptr<T>> [loplugin:useuniqueptr]}}
+    }
+};
+class Class7 {
+    std::array<int*,10> m_pbar; // expected-note {{member is here [loplugin:useuniqueptr]}}
+    ~Class7()
+    {
+        for (int i = 0; i < 10; ++i)
+            delete m_pbar[i]; // expected-error {{rather manage with std::some_container<std::unique_ptr<T>> [loplugin:useuniqueptr]}}
+    }
+};
+// don't warn for maps, MSVC 2015 has problems with mixing std::map/std::unordered_map and std::unique_ptr
+class Class8 {
+    std::unordered_map<int, int*> m_pbar;
+    ~Class8()
+    {
+        for (auto i : m_pbar)
+            delete i.second;
+    }
+};
 /* vim:set shiftwidth=4 softtabstop=4 expandtab cinoptions=b1,g0,N-s cinkeys+=0=break: */

compilerplugins/clang/useuniqueptr.cxx

@@ -38,6 +38,8 @@ public:
     bool VisitCompoundStmt(const CompoundStmt* );
 private:
     void CheckForSingleUnconditionalDelete(const CXXDestructorDecl*, const CompoundStmt* );
+    void CheckForForLoopDelete(const CXXDestructorDecl*, const CompoundStmt* );
+    void CheckForRangedLoopDelete(const CXXDestructorDecl*, const CompoundStmt* );
     void CheckForDeleteOfPOD(const CompoundStmt* );
 };
 
@@ -52,8 +54,13 @@ bool UseUniquePtr::VisitCXXDestructorDecl(const CXXDestructorDecl* destructorDec
     if (!compoundStmt)
         return true;
 
-    CheckForSingleUnconditionalDelete(destructorDecl, compoundStmt);
-    CheckForDeleteOfPOD(compoundStmt);
+    if (compoundStmt->size() > 0)
+    {
+        CheckForSingleUnconditionalDelete(destructorDecl, compoundStmt);
+        CheckForDeleteOfPOD(compoundStmt);
+        CheckForForLoopDelete(destructorDecl, compoundStmt);
+        CheckForRangedLoopDelete(destructorDecl, compoundStmt);
+    }
 
     return true;
 }
@@ -78,9 +85,15 @@ void UseUniquePtr::CheckForSingleUnconditionalDelete(const CXXDestructorDecl* de
                 deleteExpr = dyn_cast<CXXDeleteExpr>(compoundStmt->body_front());
         }
     } else {
-        return;
+        // 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 == nullptr)
+    if (!deleteExpr)
         return;
 
     const ImplicitCastExpr* pCastExpr = dyn_cast<ImplicitCastExpr>(deleteExpr->getArgument());
@@ -145,6 +158,116 @@ void UseUniquePtr::CheckForSingleUnconditionalDelete(const CXXDestructorDecl* de
         << 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 (;;)
+    {
+        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;
+            }
+            return;
+        }
+        else
+            return;
+    }
+
+    // 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;
+
+    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;
+
+    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)
 {
     if (ignoreLocation(compoundStmt))
@@ -255,7 +378,7 @@ void UseUniquePtr::CheckForDeleteOfPOD(const CompoundStmt* compoundStmt)
 
 
 
-loplugin::Plugin::Registration< UseUniquePtr > X("useuniqueptr");
+loplugin::Plugin::Registration< UseUniquePtr > X("useuniqueptr", false);
 
 }
 

i18npool/inc/breakiterator_unicode.hxx

@@ -22,6 +22,7 @@
 #include <breakiteratorImpl.hxx>
 
 #include <unicode/brkiter.h>
+#include <memory>
 
 namespace com { namespace sun { namespace star { namespace i18n {
 
@@ -74,10 +75,10 @@ protected:
     {
         OUString            aICUText;
         UText*              ut;
-        icu::BreakIterator* aBreakIterator;
+        std::unique_ptr<icu::BreakIterator> aBreakIterator;
         css::lang::Locale   maLocale;
 
-        BI_Data() : ut(nullptr), aBreakIterator(nullptr)
+        BI_Data() : ut(nullptr)
         {
         }
         ~BI_Data()

i18npool/source/breakiterator/breakiterator_unicode.cxx

@@ -49,11 +49,6 @@ BreakIterator_Unicode::BreakIterator_Unicode()
 
 BreakIterator_Unicode::~BreakIterator_Unicode()
 {
-    delete character.aBreakIterator;
-    delete sentence.aBreakIterator;
-    delete line.aBreakIterator;
-    for (BI_Data & word : words)
-        delete word.aBreakIterator;
 }
 
 /*
@@ -107,10 +102,7 @@ void SAL_CALL BreakIterator_Unicode::loadICUBreakIterator(const css::lang::Local
         rLocale.Language != icuBI->maLocale.Language ||
         rLocale.Country  != icuBI->maLocale.Country  ||
         rLocale.Variant  != icuBI->maLocale.Variant) {
-        if (icuBI->aBreakIterator) {
-            delete icuBI->aBreakIterator;
-            icuBI->aBreakIterator=nullptr;
-        }
+        icuBI->aBreakIterator.reset();
         if (rule) {
             uno::Sequence< OUString > breakRules = LocaleDataImpl::get()->getBreakIteratorRules(rLocale);
 
@@ -160,7 +152,7 @@ void SAL_CALL BreakIterator_Unicode::loadICUBreakIterator(const css::lang::Local
                     case LOAD_LINE_BREAKITERATOR: rbi->publicSetBreakType(UBRK_LINE); break;
                 }
 #endif
-                icuBI->aBreakIterator = rbi;
+                icuBI->aBreakIterator.reset( rbi );
             }
         }
 
@@ -170,16 +162,16 @@ void SAL_CALL BreakIterator_Unicode::loadICUBreakIterator(const css::lang::Local
             status = U_ZERO_ERROR;
             switch (rBreakType) {
                 case LOAD_CHARACTER_BREAKITERATOR:
-                    icuBI->aBreakIterator =  icu::BreakIterator::createCharacterInstance(icuLocale, status);
+                    icuBI->aBreakIterator.reset( icu::BreakIterator::createCharacterInstance(icuLocale, status) );
                     break;
                 case LOAD_WORD_BREAKITERATOR:
-                    icuBI->aBreakIterator =  icu::BreakIterator::createWordInstance(icuLocale, status);
+                    icuBI->aBreakIterator.reset( icu::BreakIterator::createWordInstance(icuLocale, status) );
                     break;
                 case LOAD_SENTENCE_BREAKITERATOR:
-                    icuBI->aBreakIterator = icu::BreakIterator::createSentenceInstance(icuLocale, status);
+                    icuBI->aBreakIterator.reset( icu::BreakIterator::createSentenceInstance(icuLocale, status) );
                     break;
                 case LOAD_LINE_BREAKITERATOR:
-                    icuBI->aBreakIterator = icu::BreakIterator::createLineInstance(icuLocale, status);
+                    icuBI->aBreakIterator.reset( icu::BreakIterator::createLineInstance(icuLocale, status) );
                     break;
             }
             if ( !U_SUCCESS(status) ) {

i18npool/source/localedata/LocaleNode.cxx

@@ -73,7 +73,7 @@ void LocaleNode::printR () const {
 }
 
 void LocaleNode::addChild ( LocaleNode * node) {
-    children.push_back(node);
+    children.emplace_back(node);
     node->parent = this;
 }
 
@@ -99,8 +99,6 @@ const LocaleNode * LocaleNode::findNode ( const sal_Char *name) const {
 
 LocaleNode::~LocaleNode()
 {
-    for (size_t i=0; i < children.size(); ++i)
-        delete children[i];
 }
 
 LocaleNode* LocaleNode::createNode (const OUString& name, const Reference< XAttributeList > & attr)

i18npool/source/localedata/LocaleNode.hxx

@@ -23,6 +23,7 @@
 #include <com/sun/star/xml/sax/XExtendedDocumentHandler.hpp>
 
 #include <vector>
+#include <memory>
 
 #include <com/sun/star/lang/XComponent.hpp>
 #include <com/sun/star/xml/sax/SAXParseException.hpp>
@@ -85,7 +86,7 @@ class LocaleNode
     OUString aValue;
     Attr aAttribs;
     LocaleNode * parent;
-    std::vector<LocaleNode*> children;
+    std::vector<std::unique_ptr<LocaleNode>> children;
 
 protected:
     mutable int nError;
@@ -97,7 +98,7 @@ public:
     const OUString& getValue() const { return aValue; };
     const Attr& getAttr() const { return aAttribs; };
     sal_Int32 getNumberOfChildren () const { return sal_Int32(children.size()); };
-    LocaleNode * getChildAt (sal_Int32 idx) const { return children[idx] ; };
+    LocaleNode * getChildAt (sal_Int32 idx) const { return children[idx].get(); };
     const LocaleNode * findNode ( const sal_Char *name) const;
     void print () const;
     void printR () const;