improve and enable loplugin:fragiledestructor

Where the problem was benign and the class was not extended, I marked
the class as final.
Where the problem was benign and the class was extended, I marked the
relevant callee methods as final.

Other cases were excluded in the plugin.

Change-Id: Idb762fb2206af4e8b534aa35ff77f8368c7909bc
Reviewed-on: https://gerrit.libreoffice.org/79089
Tested-by: Jenkins
Reviewed-by: Noel Grandin <noel.grandin@collabora.co.uk>

2 files changed, 160 insertions, 41 deletions

diff --git a/compilerplugins/clang/fragiledestructor.cxx b/compilerplugins/clang/fragiledestructor.cxx
index 1631cb30c1e9..a1a09a2e6081 100644
--- a/compilerplugins/clang/fragiledestructor.cxx
+++ b/compilerplugins/clang/fragiledestructor.cxx
@@ -6,6 +6,7 @@
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/.
  */
+#ifndef LO_CLANG_SHARED_PLUGINS
 
 #include <string>
 #include <iostream>
@@ -28,71 +29,99 @@ public:
     explicit FragileDestructor(loplugin::InstantiationData const & data):
         FilteringPlugin(data) {}
 
-    virtual void run() override { TraverseDecl(compiler.getASTContext().getTranslationUnitDecl()); }
+    virtual bool preRun() override
+    {
+        StringRef fn(handler.getMainFileName());
+
+        // TODO, these all need fixing
+
+        if (loplugin::isSamePathname(fn, SRCDIR "/comphelper/source/misc/proxyaggregation.cxx"))
+             return false;
+        if (loplugin::isSamePathname(fn, SRCDIR "/svx/source/svdraw/svdpntv.cxx")) // ~SdrPaintView calling ClearPageView
+             return false;
+        if (loplugin::isSamePathname(fn, SRCDIR "/svx/source/svdraw/svdobj.cxx")) // ~SdrObject calling GetLastBoundRect
+             return false;
+        if (loplugin::isSamePathname(fn, SRCDIR "/svx/source/svdraw/svdedxv.cxx")) // ~SdrObjEditView calling SdrEndTextEdit
+             return false;
+        if (loplugin::isSamePathname(fn, SRCDIR "/connectivity/source/drivers/file/FStatement.cxx")) // ~OStatement_Base calling disposing
+             return false;
+        if (loplugin::isSamePathname(fn, SRCDIR "/sd/source/core/CustomAnimationEffect.cxx")) // ~EffectSequenceHelper calling reset
+             return false;
+        if (loplugin::isSamePathname(fn, SRCDIR "/sd/source/ui/view/sdview.cxx")) // ~View calling DeleteWindowFromPaintView
+             return false;
+        if (loplugin::isSamePathname(fn, SRCDIR "/sw/source/core/layout/ssfrm.cxx")) // ~SwFrame calling IsDeleteForbidden
+             return false;
 
-    bool TraverseCXXDestructorDecl(CXXDestructorDecl *);
+        return true;
+    }
+
+    virtual void run() override
+    {
+        if (preRun())
+            TraverseDecl(compiler.getASTContext().getTranslationUnitDecl());
+    }
 
+    bool PreTraverseCXXDestructorDecl(CXXDestructorDecl*);
+    bool PostTraverseCXXDestructorDecl(CXXDestructorDecl*, bool);
+    bool TraverseCXXDestructorDecl(CXXDestructorDecl*);
     bool VisitCXXMemberCallExpr(const CXXMemberCallExpr *);
 
 private:
-    bool mbChecking = false;
+    std::vector<CXXDestructorDecl*>  m_vDestructors;
 };
 
-bool FragileDestructor::TraverseCXXDestructorDecl(CXXDestructorDecl* pCXXDestructorDecl)
+bool FragileDestructor::PreTraverseCXXDestructorDecl(CXXDestructorDecl* cxxDestructorDecl)
 {
-    if (ignoreLocation(pCXXDestructorDecl)) {
-        return RecursiveASTVisitor::TraverseCXXDestructorDecl(pCXXDestructorDecl);
-    }
-    if (!pCXXDestructorDecl->isThisDeclarationADefinition()) {
-        return RecursiveASTVisitor::TraverseCXXDestructorDecl(pCXXDestructorDecl);
-    }
-    // ignore this for now, too tricky for me to work out
-    StringRef aFileName = getFileNameOfSpellingLoc(
-            compiler.getSourceManager().getSpellingLoc(compat::getBeginLoc(pCXXDestructorDecl)));
-    if (loplugin::hasPathnamePrefix(aFileName, SRCDIR "/include/comphelper/")
-        || loplugin::hasPathnamePrefix(aFileName, SRCDIR "/include/cppuhelper/")
-        || loplugin::hasPathnamePrefix(aFileName, SRCDIR "/cppuhelper/")
-        || loplugin::hasPathnamePrefix(aFileName, SRCDIR "/comphelper/")
-        // don't know how to detect this in clang - it is making an explicit call to its own method, so presumably OK
-        || loplugin::isSamePathname(aFileName, SRCDIR "/basic/source/sbx/sbxvalue.cxx")
-       )
-        return RecursiveASTVisitor::TraverseCXXDestructorDecl(pCXXDestructorDecl);
-    mbChecking = true;
-    bool ret = RecursiveASTVisitor::TraverseCXXDestructorDecl(pCXXDestructorDecl);
-    mbChecking = false;
+    if (ignoreLocation(cxxDestructorDecl))
+        return true;
+    if (!cxxDestructorDecl->isThisDeclarationADefinition())
+        return true;
+    if (cxxDestructorDecl->getParent()->hasAttr<FinalAttr>())
+        return true;
+    m_vDestructors.push_back(cxxDestructorDecl);
+    return true;
+}
+
+bool FragileDestructor::PostTraverseCXXDestructorDecl(CXXDestructorDecl* cxxDestructorDecl, bool)
+{
+    if (!m_vDestructors.empty() && m_vDestructors.back() == cxxDestructorDecl)
+        m_vDestructors.pop_back();
+    return true;
+}
+
+bool FragileDestructor::TraverseCXXDestructorDecl(CXXDestructorDecl* cxxDestructorDecl)
+{
+    PreTraverseCXXDestructorDecl(cxxDestructorDecl);
+    auto ret = FilteringPlugin::TraverseCXXDestructorDecl(cxxDestructorDecl);
+    PostTraverseCXXDestructorDecl(cxxDestructorDecl, ret);
     return ret;
 }
 
 bool FragileDestructor::VisitCXXMemberCallExpr(const CXXMemberCallExpr* callExpr)
 {
-    if (!mbChecking || ignoreLocation(callExpr)) {
+    if (m_vDestructors.empty() || ignoreLocation(callExpr))
         return true;
-    }
     const CXXMethodDecl* methodDecl = callExpr->getMethodDecl();
-    if (!methodDecl->isVirtual() || methodDecl->hasAttr<FinalAttr>()) {
+    if (!methodDecl->isVirtual() || methodDecl->hasAttr<FinalAttr>())
         return true;
-    }
     const CXXRecordDecl* parentRecordDecl = methodDecl->getParent();
-    if (parentRecordDecl->hasAttr<FinalAttr>()) {
+    if (parentRecordDecl->hasAttr<FinalAttr>())
         return true;
-    }
-    if (!callExpr->getImplicitObjectArgument()->IgnoreImpCasts()->isImplicitCXXThis()) {
+    if (!callExpr->getImplicitObjectArgument()->IgnoreImpCasts()->isImplicitCXXThis())
         return true;
-    }
+
     // if we see an explicit call to its own method, that's OK
     auto s1 = compiler.getSourceManager().getCharacterData(compat::getBeginLoc(callExpr));
     auto s2 = compiler.getSourceManager().getCharacterData(compat::getEndLoc(callExpr));
     std::string tok(s1, s2-s1);
-    if (tok.find("::") != std::string::npos) {
+    if (tok.find("::") != std::string::npos)
         return true;
-    }
-    // e.g. osl/thread.hxx and cppuhelper/compbase.hxx
-    StringRef aFileName = getFileNameOfSpellingLoc(
-        compiler.getSourceManager().getSpellingLoc(compat::getBeginLoc(methodDecl)));
-    if (loplugin::hasPathnamePrefix(aFileName, SRCDIR "/include/osl/")
-        || loplugin::hasPathnamePrefix(aFileName, SRCDIR "/include/comphelper/")
-        || loplugin::hasPathnamePrefix(aFileName, SRCDIR "/include/cppuhelper/"))
+
+    // Very common pattern that we call acquire/dispose in destructors of UNO objects
+    // to make sure they are cleaned up.
+    if (methodDecl->getName() == "acquire" || methodDecl->getName() == "dispose")
         return true;
+
     report(
         DiagnosticsEngine::Warning,
         "calling virtual method from destructor, either make the virtual method final, or make this class final",
@@ -107,8 +136,10 @@ bool FragileDestructor::VisitCXXMemberCallExpr(const CXXMemberCallExpr* callExpr
 }
 
 
-loplugin::Plugin::Registration< FragileDestructor > X("fragiledestructor", false);
+loplugin::Plugin::Registration<FragileDestructor> fragiledestructor("fragiledestructor");
 
 }
 
+#endif // LO_CLANG_SHARED_PLUGINS
+
 /* vim:set shiftwidth=4 softtabstop=4 expandtab: */
diff --git a/compilerplugins/clang/test/fragiledestructor.cxx b/compilerplugins/clang/test/fragiledestructor.cxx
new file mode 100644
index 000000000000..e2fbfc59660d
--- /dev/null
+++ b/compilerplugins/clang/test/fragiledestructor.cxx
@@ -0,0 +1,88 @@
+/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4; fill-column: 100 -*- */
+/*
+ * This file is part of the LibreOffice project.
+ *
+ * This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/.
+ */
+
+#include "sal/config.h"
+
+// no warning expected
+namespace test1
+{
+class Foo
+{
+    ~Foo() { f(); }
+    void f();
+};
+}
+
+namespace test2
+{
+class Foo
+{
+    ~Foo() { f(); }
+    // expected-error@-1 {{calling virtual method from destructor, either make the virtual method final, or make this class final [loplugin:fragiledestructor]}}
+    virtual void f();
+    // expected-note@-1 {{callee method here [loplugin:fragiledestructor]}}
+};
+}
+
+// no warning expected
+namespace test3
+{
+class Foo final
+{
+    ~Foo() { f(); }
+    virtual void f();
+};
+}
+
+namespace test4
+{
+struct Bar
+{
+    virtual ~Bar();
+    virtual void f();
+    // expected-note@-1 {{callee method here [loplugin:fragiledestructor]}}
+};
+class Foo : public Bar
+{
+    ~Foo() { f(); }
+    // expected-error@-1 {{calling virtual method from destructor, either make the virtual method final, or make this class final [loplugin:fragiledestructor]}}
+};
+}
+
+// no warning expected
+namespace test5
+{
+struct Bar
+{
+    virtual ~Bar();
+    virtual void f();
+};
+class Foo : public Bar
+{
+    ~Foo() { f(); }
+    virtual void f() final override;
+};
+}
+
+// no warning expected
+namespace test6
+{
+struct Bar
+{
+    virtual ~Bar();
+    virtual void f();
+};
+class Foo : public Bar
+{
+    ~Foo() { Foo::f(); }
+    virtual void f() override;
+};
+}
+
+/* vim:set shiftwidth=4 softtabstop=4 expandtab cinoptions=b1,g0,N-s cinkeys+=0=break: */