loplugin: vclwidget: add check that dispose is calling superclass dispose

and fix up the places it finds

Change-Id: Ie1decd5cb14415ace423fc7a0609cc62044e19ff

8 files changed, 98 insertions, 28 deletions

diff --git a/compilerplugins/clang/vclwidgets.cxx b/compilerplugins/clang/vclwidgets.cxx
index 040c5ba2407d..402351ef73fc 100644
--- a/compilerplugins/clang/vclwidgets.cxx
+++ b/compilerplugins/clang/vclwidgets.cxx
@@ -38,6 +38,9 @@ public:
     bool VisitParmVarDecl(ParmVarDecl const * decl);
 
     bool VisitFunctionDecl( const FunctionDecl* var );
+
+private:
+    bool isDisposeCallingSuperclassDispose(const CXXMethodDecl* pMethodDecl);
 };
 
 bool BaseCheckNotWindowSubclass(const CXXRecordDecl *BaseDefinition, void *) {
@@ -115,12 +118,12 @@ bool VCLWidgets::VisitFieldDecl(const FieldDecl * fieldDecl) {
         return true;
     }
     if (isPointerToWindowSubclass(fieldDecl->getType())) {
-        report(
+/*        report(
             DiagnosticsEngine::Remark,
             "vcl::Window subclass declared as a pointer field, should be wrapped in VclPtr.",
             fieldDecl->getLocation())
           << fieldDecl->getSourceRange();
-        return true;
+        return true;*/
     }
 
     const RecordType *recordType = fieldDecl->getType()->getAs<RecordType>();
@@ -176,6 +179,10 @@ bool VCLWidgets::VisitFunctionDecl( const FunctionDecl* functionDecl )
         && pMethodDecl->getParent()->getQualifiedNameAsString().find("VclPtr") != std::string::npos) {
         return true;
     }
+    if (pMethodDecl
+        && pMethodDecl->getParent()->getQualifiedNameAsString().compare("vcl::Window") == 0) {
+        return true;
+    }
     QualType t1 { compat::getReturnType(*functionDecl) };
     if (t1.getAsString().find("VclPtr") != std::string::npos) {
         report(
@@ -184,9 +191,74 @@ bool VCLWidgets::VisitFunctionDecl( const FunctionDecl* functionDecl )
             functionDecl->getLocation())
           << functionDecl->getSourceRange();
     }
+    if (functionDecl->hasBody() && pMethodDecl && isDerivedFromWindow(pMethodDecl->getParent())) {
+        const CXXDestructorDecl *pDestructorDecl = dyn_cast<CXXDestructorDecl>(pMethodDecl);
+        // check that the destructor for a vcl::Window subclass does nothing except call into the dispose() method
+        if (pDestructorDecl) {
+            const CompoundStmt *pCompoundStatement = dyn_cast<CompoundStmt>(functionDecl->getBody());
+            bool ok = false;
+            if (pCompoundStatement && pCompoundStatement->size() == 1) {
+                const CXXMemberCallExpr *pCallExpr = dyn_cast<CXXMemberCallExpr>(*pCompoundStatement->body_begin());
+                if (pCallExpr) {
+                    ok = true;
+                }
+            }
+            if (!ok) {
+                report(
+                    DiagnosticsEngine::Warning,
+                    "vcl::Window subclass should have nothing in it's destructor but a call to dispose().",
+                    functionDecl->getBody()->getLocStart())
+                  << functionDecl->getBody()->getSourceRange();
+            }
+        // check the last thing that the dispose() method does, is to call into the superclass dispose method
+        } else if (pMethodDecl->getNameAsString() == "dispose") {
+            if (!isDisposeCallingSuperclassDispose(pMethodDecl)) {
+                report(
+                    DiagnosticsEngine::Warning,
+                    "vcl::Window subclass dispose() method MUST call it's superclass dispose() as the last thing it does.",
+                    functionDecl->getBody()->getLocStart())
+                  << functionDecl->getBody()->getSourceRange();
+           }
+        }
+    }
     return true;
 }
 
+/**
+The AST looks like:
+`-CXXMemberCallExpr 0xb06d8b0 'void'
+  `-MemberExpr 0xb06d868 '<bound member function type>' ->dispose 0x9d34880
+    `-ImplicitCastExpr 0xb06d8d8 'class SfxTabPage *' <UncheckedDerivedToBase (SfxTabPage)>
+      `-CXXThisExpr 0xb06d850 'class SfxAcceleratorConfigPage *' this
+
+*/
+bool VCLWidgets::isDisposeCallingSuperclassDispose(const CXXMethodDecl* pMethodDecl)
+{
+    const CompoundStmt *pCompoundStatement = dyn_cast<CompoundStmt>(pMethodDecl->getBody());
+    if (!pCompoundStatement) return false;
+    // find the last statement
+    const CXXMemberCallExpr *pCallExpr = dyn_cast<CXXMemberCallExpr>(*pCompoundStatement->body_rbegin());
+    if (!pCallExpr) return false;
+    const MemberExpr *pMemberExpr = dyn_cast<MemberExpr>(pCallExpr->getCallee());
+    if (!pMemberExpr) return false;
+    if (pMemberExpr->getMemberDecl()->getNameAsString() != "dispose") return false;
+    const CXXMethodDecl *pDirectCallee = dyn_cast<CXXMethodDecl>(pCallExpr->getDirectCallee());
+    if (!pDirectCallee) return false;
+/* Not working yet. Partially because sometimes the superclass does not a dispose() method, so it gets passed up the chain.
+   Need complex checking for that case.
+    if (pDirectCallee->getParent()->getTypeForDecl() != (*pMethodDecl->getParent()->bases_begin()).getType().getTypePtr()) {
+        report(
+            DiagnosticsEngine::Warning,
+            "dispose() method calling wrong baseclass, calling " + pDirectCallee->getParent()->getQualifiedNameAsString() +
+            " should be calling " + (*pMethodDecl->getParent()->bases_begin()).getType().getAsString(),
+            pCallExpr->getLocStart())
+          << pCallExpr->getSourceRange();
+        return false;
+    }*/
+    return true;
+}
+
+
 
 loplugin::Plugin::Registration< VCLWidgets > X("vclwidgets");
 
diff --git a/svtools/source/dialogs/wizardmachine.cxx b/svtools/source/dialogs/wizardmachine.cxx
index dd3b37aea806..79ffe8649fe3 100644
--- a/svtools/source/dialogs/wizardmachine.cxx
+++ b/svtools/source/dialogs/wizardmachine.cxx
@@ -55,6 +55,7 @@ namespace svt
     void OWizardPage::dispose()
     {
         delete m_pImpl;
+        TabPage::dispose();
     }
 
     void OWizardPage::initializePage()
diff --git a/vcl/inc/ilstbox.hxx b/vcl/inc/ilstbox.hxx
index 375d3cd48382..1f023a869ba3 100644
--- a/vcl/inc/ilstbox.hxx
+++ b/vcl/inc/ilstbox.hxx
@@ -403,7 +403,6 @@ protected:
     virtual void        DataChanged( const DataChangedEvent& rDCEvt ) SAL_OVERRIDE;
 
     virtual bool        Notify( NotifyEvent& rNEvt ) SAL_OVERRIDE;
-    virtual void        dispose() SAL_OVERRIDE;
 
     void                ImplResizeControls();
     void                ImplCheckScrollBars();
@@ -416,6 +415,7 @@ protected:
 public:
                     ImplListBox( vcl::Window* pParent, WinBits nWinStyle );
                     virtual ~ImplListBox();
+    virtual void    dispose() SAL_OVERRIDE;
 
     const ImplEntryList*    GetEntryList() const            { return maLBWindow->GetEntryList(); }
     ImplListBoxWindowPtr    GetMainWindow()                 { return maLBWindow; }
diff --git a/vcl/source/control/ilstbox.cxx b/vcl/source/control/ilstbox.cxx
index dd9cf5208bf9..b6a63c66ea6e 100644
--- a/vcl/source/control/ilstbox.cxx
+++ b/vcl/source/control/ilstbox.cxx
@@ -2169,9 +2169,16 @@ ImplListBox::ImplListBox( vcl::Window* pParent, WinBits nWinStyle ) :
 
 ImplListBox::~ImplListBox()
 {
+    dispose();
+}
+
+void ImplListBox::dispose()
+{
     delete mpHScrollBar;
     delete mpVScrollBar;
     delete mpScrollBarBox;
+    maLBWindow.clear();
+    Control::dispose();
 }
 
 void ImplListBox::Clear()
@@ -2521,12 +2528,6 @@ bool ImplListBox::Notify( NotifyEvent& rNEvt )
     return nDone || Window::Notify( rNEvt );
 }
 
-void ImplListBox::dispose()
-{
-    maLBWindow.clear();
-    Control::dispose();
-}
-
 const Wallpaper& ImplListBox::GetDisplayBackground() const
 {
     return maLBWindow->GetDisplayBackground();
diff --git a/vcl/source/window/dialog.cxx b/vcl/source/window/dialog.cxx
index d53c91a2c3d5..d79b5391a28e 100644
--- a/vcl/source/window/dialog.cxx
+++ b/vcl/source/window/dialog.cxx
@@ -539,13 +539,12 @@ void Dialog::settingOptimalLayoutSize(Window *pBox)
 Dialog::~Dialog()
 {
     dispose();
-
-    delete mpDialogImpl;
-    mpDialogImpl = NULL;
 }
 
 void Dialog::dispose()
 {
+    delete mpDialogImpl;
+    mpDialogImpl = NULL;
     mpActionArea.disposeAndClear();
     mpContentArea.disposeAndClear();
     SystemWindow::dispose();
diff --git a/vcl/source/window/printdlg.cxx b/vcl/source/window/printdlg.cxx
index 2c63c674093b..ead659fcac08 100644
--- a/vcl/source/window/printdlg.cxx
+++ b/vcl/source/window/printdlg.cxx
@@ -717,6 +717,7 @@ PrintDialog::~PrintDialog()
 void PrintDialog::dispose()
 {
     delete mpCustomOptionsUIBuilder;
+    ModalDialog::dispose();
 }
 
 void PrintDialog::readFromSettings()
diff --git a/vcl/source/window/wrkwin.cxx b/vcl/source/window/wrkwin.cxx
index 4cdb9a553ec3..d53718a448ca 100644
--- a/vcl/source/window/wrkwin.cxx
+++ b/vcl/source/window/wrkwin.cxx
@@ -113,17 +113,17 @@ WorkWindow::WorkWindow( SystemParentData* pParent ) :
 
 WorkWindow::~WorkWindow()
 {
+    dispose();
+}
+
+void WorkWindow::dispose()
+{
     ImplSVData* pSVData = ImplGetSVData();
     if ( pSVData->maWinData.mpAppWin == this )
     {
         pSVData->maWinData.mpAppWin = NULL;
         Application::Quit();
     }
-    dispose();
-}
-
-void WorkWindow::dispose()
-{
     SystemWindow::dispose();
 }
 
diff --git a/vcl/unx/generic/app/i18n_status.cxx b/vcl/unx/generic/app/i18n_status.cxx
index fb49388a2d82..d60cc224d46a 100644
--- a/vcl/unx/generic/app/i18n_status.cxx
+++ b/vcl/unx/generic/app/i18n_status.cxx
@@ -124,8 +124,15 @@ XIMStatusWindow::XIMStatusWindow( bool bOn ) :
 
 XIMStatusWindow::~XIMStatusWindow()
 {
+    dispose();
+}
+
+void XIMStatusWindow::dispose()
+{
     if( m_nDelayedEvent )
         Application::RemoveUserEvent( m_nDelayedEvent );
+    m_aStatusText.disposeAndClear();
+    StatusWindow::dispose();
 }
 
 void XIMStatusWindow::toggle( bool bOn )
@@ -134,12 +141,6 @@ void XIMStatusWindow::toggle( bool bOn )
     show( bOn, I18NStatus::contextmap );
 }
 
-void XIMStatusWindow::dispose()
-{
-    m_aStatusText.disposeAndClear();
-    StatusWindow::dispose();
-};
-
 void XIMStatusWindow::layout()
 {
     m_aWindowSize.Width() = m_aStatusText->GetTextWidth( m_aStatusText->GetText() )+8;
@@ -311,7 +312,6 @@ class IIIMPStatusWindow : public StatusWindow
 
 public:
     IIIMPStatusWindow( SalFrame* pParent, bool bOn ); // for initial position
-    virtual ~IIIMPStatusWindow();
 
     virtual void setText( const OUString & ) SAL_OVERRIDE;
     virtual void show( bool bShow, I18NStatus::ShowReason eReason ) SAL_OVERRIDE;
@@ -368,10 +368,6 @@ IIIMPStatusWindow::IIIMPStatusWindow( SalFrame* pParent, bool bOn ) :
     EnableAlwaysOnTop( true );
 }
 
-IIIMPStatusWindow::~IIIMPStatusWindow()
-{
-}
-
 void IIIMPStatusWindow::layout()
 {
     Font aFont( m_aStatusBtn->GetFont() );