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 <noel.grandin@collabora.co.uk>

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: */

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);
 
 }

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();
 

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;
 

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_;
     };

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()

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;
         }

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 );

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 );
 

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: */