[Libreoffice-commits] core.git: compilerplugins/clang i18npool/inc i18npool/source
Noel Grandin
noel.grandin at collabora.co.uk
Wed Aug 23 16:37:24 UTC 2017
compilerplugins/clang/test/useuniqueptr.cxx | 45 +++++
compilerplugins/clang/useuniqueptr.cxx | 133 +++++++++++++++-
i18npool/inc/breakiterator_unicode.hxx | 5
i18npool/source/breakiterator/breakiterator_unicode.cxx | 20 --
i18npool/source/localedata/LocaleNode.cxx | 4
i18npool/source/localedata/LocaleNode.hxx | 5
6 files changed, 186 insertions(+), 26 deletions(-)
New commits:
commit c66568d6b0bbcce26cbdc5a4e5f6e4c0ae748e45
Author: Noel Grandin <noel.grandin at collabora.co.uk>
Date: Wed Aug 23 08:49:02 2017 +0200
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/41438
Reviewed-by: Noel Grandin <noel.grandin at collabora.co.uk>
Tested-by: Noel Grandin <noel.grandin at collabora.co.uk>
diff --git a/compilerplugins/clang/test/useuniqueptr.cxx b/compilerplugins/clang/test/useuniqueptr.cxx
index 7e64bcca8986..b45781925630 100644
--- a/compilerplugins/clang/test/useuniqueptr.cxx
+++ b/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: */
diff --git a/compilerplugins/clang/useuniqueptr.cxx b/compilerplugins/clang/useuniqueptr.cxx
index 977404ab2ae6..e47ebd23450a 100644
--- a/compilerplugins/clang/useuniqueptr.cxx
+++ b/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);
}
diff --git a/i18npool/inc/breakiterator_unicode.hxx b/i18npool/inc/breakiterator_unicode.hxx
index 42dcb305777a..8783764cb0e1 100644
--- a/i18npool/inc/breakiterator_unicode.hxx
+++ b/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()
diff --git a/i18npool/source/breakiterator/breakiterator_unicode.cxx b/i18npool/source/breakiterator/breakiterator_unicode.cxx
index 2256755e85be..cf781eb414a0 100644
--- a/i18npool/source/breakiterator/breakiterator_unicode.cxx
+++ b/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) ) {
diff --git a/i18npool/source/localedata/LocaleNode.cxx b/i18npool/source/localedata/LocaleNode.cxx
index a17ff49cefcf..5d3e29a3531c 100644
--- a/i18npool/source/localedata/LocaleNode.cxx
+++ b/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)
diff --git a/i18npool/source/localedata/LocaleNode.hxx b/i18npool/source/localedata/LocaleNode.hxx
index c3e6c57e6a22..0f089d162bf3 100644
--- a/i18npool/source/localedata/LocaleNode.hxx
+++ b/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;
More information about the Libreoffice-commits
mailing list