loplugin: vclwidget: add check that dispose is calling superclass dispose
and fix up the places it finds
Change-Id: Ie1decd5cb14415ace423fc7a0609cc62044e19ff
diff --git a/compilerplugins/clang/vclwidgets.cxx b/compilerplugins/clang/vclwidgets.cxx
index 040c5ba..402351e 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 dd3b37a..79ffe86 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 375d3cd..1f023a8 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 dd9cf52..b6a63c6 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 d53c91a..d79b539 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 2c63c67..ead659f 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 4cdb9a5..d53718a 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 fb49388..d60cc22 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() );
