compilerplugins/clang/test/useuniqueptr.cxx | 4 compilerplugins/clang/useuniqueptr.cxx | 62 ++++++++++++++ include/svx/gallery1.hxx | 9 -- include/tools/config.hxx | 3 shell/source/unix/sysshell/recently_used_file_handler.cxx | 42 ++------- svx/source/gengal/gengal.cxx | 20 ---- sw/source/filter/xml/xmlimpit.cxx | 40 +++------ sw/source/filter/xml/xmlithlp.cxx | 15 +-- sw/source/filter/xml/xmlithlp.hxx | 4 tools/source/generic/config.cxx | 22 +--- 10 files changed, 114 insertions(+), 107 deletions(-)
New commits: commit 200ca388246e525f6e8f909766977f534c0c897e Author: Noel Grandin <[email protected]> Date: Thu Jun 21 15:32:52 2018 +0200 teach useuniqueptr loplugin about calling delete on a param which is often a useful indicator that the callers can be made to use std::unique_ptr Change-Id: Idb1745d1f58dbcf389c9f60f1a5728d7d092ade5 Reviewed-on: https://gerrit.libreoffice.org/56238 Tested-by: Jenkins Reviewed-by: Noel Grandin <[email protected]> diff --git a/compilerplugins/clang/test/useuniqueptr.cxx b/compilerplugins/clang/test/useuniqueptr.cxx index 8ef71175bb99..c7c92313a5e6 100644 --- a/compilerplugins/clang/test/useuniqueptr.cxx +++ b/compilerplugins/clang/test/useuniqueptr.cxx @@ -172,4 +172,8 @@ class Foo14 { } } }; +void Foo15(int * p) +{ + delete p; // expected-error {{calling delete on a pointer param, should be either whitelisted here or simplified [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 2424bcca8c09..131727f0f44f 100644 --- a/compilerplugins/clang/useuniqueptr.cxx +++ b/compilerplugins/clang/useuniqueptr.cxx @@ -68,6 +68,8 @@ public: bool VisitCXXMethodDecl(const CXXMethodDecl* ); bool VisitCompoundStmt(const CompoundStmt* ); + bool VisitCXXDeleteExpr(const CXXDeleteExpr* ); + bool TraverseFunctionDecl(FunctionDecl* ); private: void CheckCompoundStmt(const CXXMethodDecl*, const CompoundStmt* ); void CheckForUnconditionalDelete(const CXXMethodDecl*, const CompoundStmt* ); @@ -79,6 +81,7 @@ private: void CheckParenExpr(const CXXMethodDecl*, const ParenExpr*); void CheckDeleteExpr(const CXXMethodDecl*, const CXXDeleteExpr*, const MemberExpr*, StringRef message); + FunctionDecl const * mpCurrentFunctionDecl = nullptr; }; bool UseUniquePtr::VisitCXXMethodDecl(const CXXMethodDecl* methodDecl) @@ -413,6 +416,65 @@ bool UseUniquePtr::VisitCompoundStmt(const CompoundStmt* compoundStmt) return true; } +bool UseUniquePtr::TraverseFunctionDecl(FunctionDecl* functionDecl) +{ + if (ignoreLocation(functionDecl)) + return true; + + auto oldCurrent = mpCurrentFunctionDecl; + mpCurrentFunctionDecl = functionDecl; + bool ret = RecursiveASTVisitor::TraverseFunctionDecl(functionDecl); + mpCurrentFunctionDecl = oldCurrent; + + return ret; +} + +bool UseUniquePtr::VisitCXXDeleteExpr(const CXXDeleteExpr* deleteExpr) +{ + if (!mpCurrentFunctionDecl) + return true; + if (ignoreLocation(mpCurrentFunctionDecl)) + return true; + if (isInUnoIncludeFile(mpCurrentFunctionDecl->getCanonicalDecl()->getLocStart())) + return true; + if (mpCurrentFunctionDecl->getIdentifier()) + { + auto name = mpCurrentFunctionDecl->getName(); + if (name == "delete_IncludesCollection" || name == "convertName" + || name == "createNamedType" + || name == "typelib_typedescriptionreference_release" || name == "deleteExceptions" + || name == "uno_threadpool_destroy" + || name == "AddRanges_Impl" + || name == "DestroySalInstance" + || name == "ImplHandleUserEvent" + || name == "releaseDecimalPtr" // TODO, basic + || name == "replaceAndReset" // TODO, connectivity + || name == "intrusive_ptr_release" + || name == "FreeParaList" + || name == "DeleteSdrUndoAction" // TODO, sc + || name == "lcl_MergeGCBox" || name == "lcl_MergeGCLine" || name == "lcl_DelHFFormat") + return true; + } + + auto declRefExpr = dyn_cast<DeclRefExpr>(deleteExpr->getArgument()->IgnoreParenImpCasts()); + if (!declRefExpr) + return true; + auto varDecl = dyn_cast<ParmVarDecl>(declRefExpr->getDecl()); + if (!varDecl) + return true; + + /* + Sometimes we can pass the param as std::unique_ptr<T>& or std::unique_ptr, sometimes the method + just needs to be inlined, which normally exposes more simplification. + */ + report( + DiagnosticsEngine::Warning, + "calling delete on a pointer param, should be either whitelisted here or simplified", + deleteExpr->getLocStart()) + << deleteExpr->getSourceRange(); + return true; +} + loplugin::Plugin::Registration< UseUniquePtr > X("useuniqueptr", false); } diff --git a/include/svx/gallery1.hxx b/include/svx/gallery1.hxx index 66474d17180b..989603e46deb 100644 --- a/include/svx/gallery1.hxx +++ b/include/svx/gallery1.hxx @@ -86,10 +86,6 @@ class GalleryThemeCacheEntry; class SVX_DLLPUBLIC Gallery : public SfxBroadcaster { - // only for gengal utility! - friend Gallery* createGallery( const OUString& ); - friend void disposeGallery( Gallery* ); - typedef std::vector<GalleryThemeCacheEntry*> GalleryCacheThemeList; private: @@ -108,12 +104,13 @@ private: SAL_DLLPRIVATE GalleryTheme* ImplGetCachedTheme( const GalleryThemeEntry* pThemeEntry ); SAL_DLLPRIVATE void ImplDeleteCachedTheme( GalleryTheme const * pTheme ); - Gallery( const OUString& rMultiPath ); - virtual ~Gallery() override; Gallery& operator=( Gallery const & ) = delete; // MSVC2015 workaround Gallery( Gallery const & ) = delete; // MSVC2015 workaround public: + // only for gengal utility! + Gallery( const OUString& rMultiPath ); + virtual ~Gallery() override; static Gallery* GetGalleryInstance(); diff --git a/include/tools/config.hxx b/include/tools/config.hxx index d6a00aa61d64..76e4270b5e9d 100644 --- a/include/tools/config.hxx +++ b/include/tools/config.hxx @@ -21,6 +21,7 @@ #include <tools/toolsdllapi.h> #include <rtl/ustring.hxx> +#include <memory> struct ImplConfigData; struct ImplGroupData; @@ -30,7 +31,7 @@ class SAL_WARN_UNUSED TOOLS_DLLPUBLIC Config private: OUString maFileName; OString maGroupName; - ImplConfigData* mpData; + std::unique_ptr<ImplConfigData> mpData; ImplGroupData* mpActGroup; sal_uInt32 mnDataUpdateId; diff --git a/shell/source/unix/sysshell/recently_used_file_handler.cxx b/shell/source/unix/sysshell/recently_used_file_handler.cxx index 6ca13932143c..ced51eda9787 100644 --- a/shell/source/unix/sysshell/recently_used_file_handler.cxx +++ b/shell/source/unix/sysshell/recently_used_file_handler.cxx @@ -32,6 +32,7 @@ #include <i_xml_parser_event_handler.hxx> #include <map> +#include <memory> #include <vector> #include <algorithm> #include <string.h> @@ -194,7 +195,7 @@ namespace /* private */ { string_container_t groups_; }; - typedef std::vector<recently_used_item*> recently_used_item_list_t; + typedef std::vector<std::unique_ptr<recently_used_item>> recently_used_item_list_t; typedef void (recently_used_item::* SET_COMMAND)(const string_t&); // thrown if we encounter xml tags that we do not know @@ -205,7 +206,6 @@ namespace /* private */ { { public: explicit recently_used_file_filter(recently_used_item_list_t& item_list) : - item_(nullptr), item_list_(item_list) { named_command_map_[TAG_RECENT_FILES] = &recently_used_item::set_nothing; @@ -227,7 +227,7 @@ namespace /* private */ { const xml_tag_attribute_container_t& /*attributes*/) override { if ((local_name == TAG_RECENT_ITEM) && (nullptr == item_)) - item_ = new recently_used_item; + item_.reset(new recently_used_item); } virtual void end_element(const string_t& /*raw_name*/, const string_t& local_name) override @@ -237,17 +237,16 @@ namespace /* private */ { return; // will result in an XML parser error anyway if (named_command_map_.find(local_name) != named_command_map_.end()) - (item_->*named_command_map_[local_name])(current_element_); + (item_.get()->*named_command_map_[local_name])(current_element_); else { - delete item_; + item_.reset(); throw unknown_xml_format_exception(); } if (local_name == TAG_RECENT_ITEM) { - item_list_.push_back(item_); - item_ = nullptr; + item_list_.push_back(std::move(item_)); } current_element_.clear(); } @@ -264,7 +263,7 @@ namespace /* private */ { virtual void comment(const string_t& /*comment*/) override {} private: - recently_used_item* item_; + std::unique_ptr<recently_used_item> item_; std::map<string_t, SET_COMMAND> named_command_map_; string_t current_element_; recently_used_item_list_t& item_list_; @@ -301,7 +300,7 @@ namespace /* private */ { items_written_(0) {} - void operator() (const recently_used_item* item) + void operator() (const std::unique_ptr<recently_used_item> & item) { if (items_written_++ < MAX_RECENTLY_USED_ITEMS) item->write_xml(file_); @@ -338,23 +337,6 @@ namespace /* private */ { } - struct delete_recently_used_item - { - void operator() (const recently_used_item* item) const - { delete item; } - }; - - - void recently_used_item_list_clear(recently_used_item_list_t& item_list) - { - std::for_each( - item_list.begin(), - item_list.end(), - delete_recently_used_item()); - item_list.clear(); - } - - class find_item_predicate { public: @@ -362,7 +344,7 @@ namespace /* private */ { uri_(uri) {} - bool operator() (const recently_used_item* item) const + bool operator() (const std::unique_ptr<recently_used_item> & item) const { return (item->uri_ == uri_); } private: string_t uri_; @@ -371,7 +353,7 @@ namespace /* private */ { struct greater_recently_used_item { - bool operator ()(const recently_used_item* lhs, const recently_used_item* rhs) const + bool operator ()(const std::unique_ptr<recently_used_item> & lhs, const std::unique_ptr<recently_used_item> & rhs) const { return (lhs->timestamp_ > rhs->timestamp_); } }; @@ -416,7 +398,7 @@ namespace /* private */ { if (mimetype.length() == 0) mimetype = "application/octet-stream"; - item_list.push_back(new recently_used_item(uri, mimetype, groups)); + item_list.emplace_back(new recently_used_item(uri, mimetype, groups)); } // sort decreasing after the timestamp @@ -434,7 +416,7 @@ namespace /* private */ { item_list_(item_list) {} ~cleanup_guard() - { recently_used_item_list_clear(item_list_); } + { item_list_.clear(); } recently_used_item_list_t& item_list_; }; diff --git a/svx/source/gengal/gengal.cxx b/svx/source/gengal/gengal.cxx index c4654bd580da..1a218d1d0524 100644 --- a/svx/source/gengal/gengal.cxx +++ b/svx/source/gengal/gengal.cxx @@ -56,28 +56,12 @@ protected: void DeInit() override; }; -Gallery* createGallery( const OUString& rURL ) -{ - return new Gallery( rURL ); -} - -void disposeGallery( Gallery* pGallery ) -{ - delete pGallery; -} - static void createTheme( const OUString& aThemeName, const OUString& aGalleryURL, const OUString& aDestDir, std::vector<INetURLObject> &rFiles, bool bRelativeURLs ) { - Gallery* pGallery; + std::unique_ptr<Gallery> pGallery(new Gallery( aGalleryURL )); - pGallery = createGallery( aGalleryURL ); - if (!pGallery ) { - fprintf( stderr, "Couldn't create '%s'\n", - OUStringToOString( aGalleryURL, RTL_TEXTENCODING_UTF8 ).getStr() ); - exit( 1 ); - } fprintf( stderr, "Work on gallery '%s'\n", OUStringToOString( aGalleryURL, RTL_TEXTENCODING_UTF8 ).getStr() ); @@ -127,8 +111,6 @@ static void createTheme( const OUString& aThemeName, const OUString& aGalleryURL } pGallery->ReleaseTheme( pGalTheme, aListener ); - - disposeGallery( pGallery ); } static int PrintHelp() diff --git a/sw/source/filter/xml/xmlimpit.cxx b/sw/source/filter/xml/xmlimpit.cxx index 0c9566012c26..d6b1ea06ad44 100644 --- a/sw/source/filter/xml/xmlimpit.cxx +++ b/sw/source/filter/xml/xmlimpit.cxx @@ -224,32 +224,24 @@ SvXMLImportItemMapper::finished(SfxItemSet &, SvXMLUnitConverter const&) const struct BoxHolder { - SvxBorderLine* pTop; - SvxBorderLine* pBottom; - SvxBorderLine* pLeft; - SvxBorderLine* pRight; + std::unique_ptr<SvxBorderLine> pTop; + std::unique_ptr<SvxBorderLine> pBottom; + std::unique_ptr<SvxBorderLine> pLeft; + std::unique_ptr<SvxBorderLine> pRight; BoxHolder(BoxHolder const&) = delete; BoxHolder& operator=(BoxHolder const&) = delete; explicit BoxHolder(SvxBoxItem const & rBox) { - pTop = rBox.GetTop() == nullptr ? - nullptr : new SvxBorderLine( *rBox.GetTop() ); - pBottom = rBox.GetBottom() == nullptr ? - nullptr : new SvxBorderLine( *rBox.GetBottom() ); - pLeft = rBox.GetLeft() == nullptr ? - nullptr : new SvxBorderLine( *rBox.GetLeft() ); - pRight = rBox.GetRight() == nullptr ? - nullptr : new SvxBorderLine( *rBox.GetRight() ); - } - - ~BoxHolder() - { - delete pTop; - delete pBottom; - delete pLeft; - delete pRight; + if (rBox.GetTop()) + pTop.reset(new SvxBorderLine( *rBox.GetTop() )); + if (rBox.GetBottom()) + pBottom.reset(new SvxBorderLine( *rBox.GetBottom() )); + if (rBox.GetLeft()) + pLeft.reset(new SvxBorderLine( *rBox.GetLeft() )); + if (rBox.GetRight()) + pRight.reset(new SvxBorderLine( *rBox.GetRight() )); } }; @@ -576,10 +568,10 @@ bool SvXMLImportItemMapper::PutXMLValue( break; } - rBox.SetLine( aBoxes.pTop, SvxBoxItemLine::TOP ); - rBox.SetLine( aBoxes.pBottom, SvxBoxItemLine::BOTTOM ); - rBox.SetLine( aBoxes.pLeft, SvxBoxItemLine::LEFT ); - rBox.SetLine( aBoxes.pRight, SvxBoxItemLine::RIGHT ); + rBox.SetLine( aBoxes.pTop.get(), SvxBoxItemLine::TOP ); + rBox.SetLine( aBoxes.pBottom.get(), SvxBoxItemLine::BOTTOM ); + rBox.SetLine( aBoxes.pLeft.get(), SvxBoxItemLine::LEFT ); + rBox.SetLine( aBoxes.pRight.get(), SvxBoxItemLine::RIGHT ); bOk = true; } diff --git a/sw/source/filter/xml/xmlithlp.cxx b/sw/source/filter/xml/xmlithlp.cxx index 4d7a5f3e09c6..d879292d5aba 100644 --- a/sw/source/filter/xml/xmlithlp.cxx +++ b/sw/source/filter/xml/xmlithlp.cxx @@ -140,7 +140,7 @@ void sw_frmitems_setXMLBorderStyle( SvxBorderLine& rLine, sal_uInt16 nStyle ) rLine.SetBorderLineStyle(eStyle); } -bool sw_frmitems_setXMLBorder( SvxBorderLine*& rpLine, +bool sw_frmitems_setXMLBorder( std::unique_ptr<SvxBorderLine>& rpLine, bool bHasStyle, sal_uInt16 nStyle, bool bHasWidth, sal_uInt16 nWidth, sal_uInt16 nNamedWidth, @@ -151,12 +151,7 @@ bool sw_frmitems_setXMLBorder( SvxBorderLine*& rpLine, (bHasWidth && USHRT_MAX == nNamedWidth && 0 == nWidth) ) { bool bRet = nullptr != rpLine; - if( rpLine ) - { - delete rpLine; - rpLine = nullptr; - } - + rpLine.reset(); return bRet; } @@ -166,7 +161,7 @@ bool sw_frmitems_setXMLBorder( SvxBorderLine*& rpLine, // We now do know that there will be a line if( !rpLine ) - rpLine = new SvxBorderLine; + rpLine.reset(new SvxBorderLine); if( ( bHasWidth && (USHRT_MAX != nNamedWidth || (nWidth != rpLine->GetWidth() ) ) ) || @@ -208,12 +203,12 @@ bool sw_frmitems_setXMLBorder( SvxBorderLine*& rpLine, return true; } -void sw_frmitems_setXMLBorder( SvxBorderLine*& rpLine, +void sw_frmitems_setXMLBorder( std::unique_ptr<SvxBorderLine>& rpLine, sal_uInt16 nWidth, sal_uInt16 nOutWidth, sal_uInt16 nInWidth, sal_uInt16 nDistance ) { if( !rpLine ) - rpLine = new SvxBorderLine; + rpLine.reset(new SvxBorderLine); if( nWidth > 0 ) rpLine->SetWidth( nWidth ); diff --git a/sw/source/filter/xml/xmlithlp.hxx b/sw/source/filter/xml/xmlithlp.hxx index c3c0512f16b8..e5498009bd3e 100644 --- a/sw/source/filter/xml/xmlithlp.hxx +++ b/sw/source/filter/xml/xmlithlp.hxx @@ -40,13 +40,13 @@ bool sw_frmitems_parseXMLBorder( const OUString& rValue, sal_uInt16& rNamedWidth, bool& rHasColor, Color& rColor ); -bool sw_frmitems_setXMLBorder( editeng::SvxBorderLine*& rpLine, +bool sw_frmitems_setXMLBorder( std::unique_ptr<editeng::SvxBorderLine>& rpLine, bool bHasStyle, sal_uInt16 nStyle, bool bHasWidth, sal_uInt16 nWidth, sal_uInt16 nNamedWidth, bool bHasColor, const Color& rColor ); -void sw_frmitems_setXMLBorder( editeng::SvxBorderLine*& rpLine, +void sw_frmitems_setXMLBorder( std::unique_ptr<editeng::SvxBorderLine>& rpLine, sal_uInt16 nWidth, sal_uInt16 nOutWidth, sal_uInt16 nInWidth, sal_uInt16 nDistance ); diff --git a/tools/source/generic/config.cxx b/tools/source/generic/config.cxx index 55ce2bdc03b8..169f6e6455aa 100644 --- a/tools/source/generic/config.cxx +++ b/tools/source/generic/config.cxx @@ -578,35 +578,27 @@ static void ImplDeleteConfigData( ImplConfigData* pData ) pData->mpFirstGroup = nullptr; } -static ImplConfigData* ImplGetConfigData( const OUString& rFileName ) +static std::unique_ptr<ImplConfigData> ImplGetConfigData( const OUString& rFileName ) { - ImplConfigData* pData; - - pData = new ImplConfigData; + std::unique_ptr<ImplConfigData> pData(new ImplConfigData); pData->maFileName = rFileName; pData->mpFirstGroup = nullptr; pData->mnDataUpdateId = 0; pData->meLineEnd = LINEEND_CRLF; pData->mbRead = false; pData->mbIsUTF8BOM = false; - ImplReadConfig( pData ); + ImplReadConfig( pData.get() ); return pData; } -static void ImplFreeConfigData( ImplConfigData* pDelData ) -{ - ImplDeleteConfigData( pDelData ); - delete pDelData; -} - bool Config::ImplUpdateConfig() const { // Re-read file if timestamp differs if ( mpData->mnTimeStamp != ImplSysGetConfigTimeStamp( maFileName ) ) { - ImplDeleteConfigData( mpData ); - ImplReadConfig( mpData ); + ImplDeleteConfigData( mpData.get() ); + ImplReadConfig( mpData.get() ); mpData->mnDataUpdateId++; return true; } @@ -667,7 +659,7 @@ Config::~Config() SAL_INFO("tools.generic", "Config::~Config()" ); Flush(); - ImplFreeConfigData( mpData ); + ImplDeleteConfigData( mpData.get() ); } void Config::SetGroup(const OString& rGroup) @@ -973,7 +965,7 @@ OString Config::ReadKey(sal_uInt16 nKey) const void Config::Flush() { if ( mpData->mbModified ) - ImplWriteConfig( mpData ); + ImplWriteConfig( mpData.get() ); } /* vim:set shiftwidth=4 softtabstop=4 expandtab: */ _______________________________________________ Libreoffice-commits mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/libreoffice-commits
