improve "unnecessary user-declared destructor" check

to look for inline&empty destructors, where we can just let
the compiler do it's thing

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

8 files changed, 73 insertions, 26 deletions

diff --git a/compilerplugins/clang/test/unnecessaryoverride-dtor.cxx b/compilerplugins/clang/test/unnecessaryoverride-dtor.cxx
index 26b3a81abd68..18f7aed6e4d8 100644
--- a/compilerplugins/clang/test/unnecessaryoverride-dtor.cxx
+++ b/compilerplugins/clang/test/unnecessaryoverride-dtor.cxx
@@ -16,7 +16,7 @@
 struct NonVirtualBase {};
 
 struct NonVirtualDerived1: NonVirtualBase {
-    ~NonVirtualDerived1() {}
+    ~NonVirtualDerived1() {} // expected-error {{unnecessary user-declared destructor [loplugin:unnecessaryoverride]}}
 };
 
 struct NonVirtualDerived2: NonVirtualBase {
@@ -41,6 +41,10 @@ IncludedDerived3::IncludedDerived3() {}
 
 IncludedDerived3::~IncludedDerived3() {}
 
+// vmiklos likes these because he can quickly add a DEBUG or something similar without
+// massive recompile
+IncludedNotDerived::~IncludedNotDerived() {}
+
 struct NoExSpecDerived: VirtualBase {
     ~NoExSpecDerived() override {} // expected-error {{unnecessary user-declared destructor [loplugin:unnecessaryoverride]}}
 };
@@ -113,9 +117,20 @@ struct PureDerived: PureBase {
     ~PureDerived() override {} // expected-error {{unnecessary user-declared destructor [loplugin:unnecessaryoverride]}}
 };
 
-// aovid loplugin:unreffun:
+struct CompleteBase {
+    ~CompleteBase() {} // expected-error {{unnecessary user-declared destructor [loplugin:unnecessaryoverride]}}
+};
+
+// <sberg> noelgrandin, there's one other corner case one can imagine:
+// a class defined in a .hxx with the dtor declared (but not defined) as inline in the .hxx,
+// and then defined in the cxx (making it effectively only callable from within the cxx);
+// removing the dtor declaration from the class definition would change the dtor to be callable from everywhere
+MarkedInlineButNotDefined::~MarkedInlineButNotDefined() {}
+
+// avoid loplugin:unreffun:
 int main() {
     (void) NonVirtualDerived1();
+    (void) CompleteBase();
 }
 
 /* vim:set shiftwidth=4 softtabstop=4 expandtab cinoptions=b1,g0,N-s cinkeys+=0=break: */
diff --git a/compilerplugins/clang/test/unnecessaryoverride-dtor.hxx b/compilerplugins/clang/test/unnecessaryoverride-dtor.hxx
index fe1f9cb180c3..81733a92a3ab 100644
--- a/compilerplugins/clang/test/unnecessaryoverride-dtor.hxx
+++ b/compilerplugins/clang/test/unnecessaryoverride-dtor.hxx
@@ -19,13 +19,17 @@ struct VirtualBase {
 };
 
 struct IncludedDerived1: VirtualBase {
-    ~IncludedDerived1() override {};
+    ~IncludedDerived1() override {}; // expected-error {{unnecessary user-declared destructor [loplugin:unnecessaryoverride]}}
 };
 
 struct IncludedDerived2: VirtualBase {
     ~IncludedDerived2() override;
 };
 
+struct IncludedNotDerived {
+    ~IncludedNotDerived();
+};
+
 struct Incomplete;
 struct IncludedDerived3: VirtualBase {
     IncludedDerived3();
@@ -38,6 +42,10 @@ private:
     rtl::Reference<Incomplete> m;
 };
 
+struct MarkedInlineButNotDefined {
+    inline ~MarkedInlineButNotDefined();
+};
+
 #endif
 
 /* vim:set shiftwidth=4 softtabstop=4 expandtab cinoptions=b1,g0,N-s cinkeys+=0=break: */
diff --git a/compilerplugins/clang/unnecessaryoverride.cxx b/compilerplugins/clang/unnecessaryoverride.cxx
index 98b77d5a7b7a..8cf811b58415 100644
--- a/compilerplugins/clang/unnecessaryoverride.cxx
+++ b/compilerplugins/clang/unnecessaryoverride.cxx
@@ -78,14 +78,25 @@ bool UnnecessaryOverride::VisitCXXMethodDecl(const CXXMethodDecl* methodDecl)
         return true;
     }
 
-    if (isa<CXXDestructorDecl>(methodDecl)) {
-        // Warn about unnecessarily user-declared overriding virtual
-        // destructors; such a destructor is deemed unnecessary if
+    StringRef aFileName = compiler.getSourceManager().getFilename(compiler.getSourceManager().getSpellingLoc(methodDecl->getLocStart()));
+
+    if (isa<CXXDestructorDecl>(methodDecl)
+       && !isInUnoIncludeFile(methodDecl))
+    {
+        // the code is this method is only __compiled__ if OSL_DEBUG_LEVEL > 1
+        if (aFileName == SRCDIR "/tools/source/stream/strmunx.cxx")
+            return true;
+
+        // Warn about unnecessarily user-declared destructors.
+        // A destructor is deemed unnecessary if:
         // * it is public;
         // * its class is only defined in the .cxx file (i.e., the virtual
-        //   destructor is neither used to controll the place of vtable
+        //   destructor is neither used to control the place of vtable
         //   emission, nor is its definition depending on types that may still
         //   be incomplete);
+        //     or
+        //   the destructor is inline, the class definition is complete,
+        //     and the class has no superclasses
         // * it either does not have an explicit exception specification, or has
         //   a non-dependent explicit exception specification that is compatible
         //   with a non-dependent exception specification the destructor would
@@ -102,14 +113,42 @@ bool UnnecessaryOverride::VisitCXXMethodDecl(const CXXMethodDecl* methodDecl)
         // implicit definition of a copy constructor and/or copy assignment
         // operator to change from being an obsolete feature to being a standard
         // feature.  That difference is not taken into account here.
-        if ((methodDecl->begin_overridden_methods()
-             == methodDecl->end_overridden_methods())
-            || methodDecl->getAccess() != AS_public)
+        auto cls = methodDecl->getParent();
+        if (methodDecl->isVirtual() && cls->getNumBases() == 0)
+        {
+            return true;
+        }
+        if (methodDecl->getAccess() != AS_public)
         {
             return true;
         }
         if (!compiler.getSourceManager().isInMainFile(
-                methodDecl->getCanonicalDecl()->getLocation()))
+                methodDecl->getCanonicalDecl()->getLocation())
+            && !( methodDecl->isInlined()))
+        {
+            return true;
+        }
+        // if it's virtual, but it has a base-class with a non-virtual destructor
+        if (methodDecl->isVirtual())
+        {
+            for (auto baseSpecifier = cls->bases_begin(); baseSpecifier != cls->bases_end(); ++baseSpecifier)
+            {
+                const RecordType* baseRecordType = baseSpecifier->getType()->getAs<RecordType>();
+                if (baseRecordType)
+                {
+                    const CXXRecordDecl* baseRecordDecl = dyn_cast<CXXRecordDecl>(baseRecordType->getDecl());
+                    if (baseRecordDecl && baseRecordDecl->getDestructor()
+                        && !baseRecordDecl->getDestructor()->isVirtual())
+                    {
+                        return true;
+                    }
+                }
+            }
+        }
+        // corner case
+        if (methodDecl->isInlined()
+            && compiler.getSourceManager().isInMainFile(methodDecl->getLocation())
+            && !compiler.getSourceManager().isInMainFile(methodDecl->getCanonicalDecl()->getLocation()))
         {
             return true;
         }
@@ -123,7 +162,6 @@ bool UnnecessaryOverride::VisitCXXMethodDecl(const CXXMethodDecl* methodDecl)
             }
         }
         //TODO: exception specification
-        auto cls = methodDecl->getParent();
         if (!(cls->hasUserDeclaredCopyConstructor()
               || cls->hasUserDeclaredCopyAssignment()
               || cls->hasUserDeclaredMoveConstructor()
@@ -166,7 +204,6 @@ bool UnnecessaryOverride::VisitCXXMethodDecl(const CXXMethodDecl* methodDecl)
         }
     }
     // sometimes the disambiguation happens in a base class
-    StringRef aFileName = compiler.getSourceManager().getFilename(compiler.getSourceManager().getSpellingLoc(methodDecl->getLocStart()));
     if (aFileName == SRCDIR "/comphelper/source/property/propertycontainer.cxx")
         return true;
     // not sure what is happening here
diff --git a/extensions/source/propctrlr/enumrepresentation.hxx b/extensions/source/propctrlr/enumrepresentation.hxx
index 309af3c6f745..550d0dbcad0b 100644
--- a/extensions/source/propctrlr/enumrepresentation.hxx
+++ b/extensions/source/propctrlr/enumrepresentation.hxx
@@ -55,7 +55,6 @@ namespace pcr
                 const css::uno::Any& _rEnumValue
             ) const = 0;
 
-        virtual ~IPropertyEnumRepresentation() override { };
     };
 
 
diff --git a/include/basegfx/raster/bpixelraster.hxx b/include/basegfx/raster/bpixelraster.hxx
index 46ef649b9a7e..491f88e75543 100644
--- a/include/basegfx/raster/bpixelraster.hxx
+++ b/include/basegfx/raster/bpixelraster.hxx
@@ -57,10 +57,6 @@ namespace basegfx
             reset();
         }
 
-        ~BPixelRaster()
-        {
-        }
-
         // coordinate calcs between X/Y and span
         sal_uInt32 getIndexFromXY(sal_uInt32 nX, sal_uInt32 nY) const { return (nX + (nY * mnWidth)); }
 
diff --git a/include/basegfx/raster/bzpixelraster.hxx b/include/basegfx/raster/bzpixelraster.hxx
index 3e0d51b99d38..e48f32fbb686 100644
--- a/include/basegfx/raster/bzpixelraster.hxx
+++ b/include/basegfx/raster/bzpixelraster.hxx
@@ -41,10 +41,6 @@ namespace basegfx
             memset(mpZBuffer.get(), 0, sizeof(sal_uInt16) * mnCount);
         }
 
-        ~BZPixelRaster()
-        {
-        }
-
         // data access read only
         const sal_uInt16& getZ(sal_uInt32 nIndex) const
         {
diff --git a/shell/source/sessioninstall/SyncDbusSessionHelper.hxx b/shell/source/sessioninstall/SyncDbusSessionHelper.hxx
index 2bb0ff741334..731b649d723a 100644
--- a/shell/source/sessioninstall/SyncDbusSessionHelper.hxx
+++ b/shell/source/sessioninstall/SyncDbusSessionHelper.hxx
@@ -21,7 +21,6 @@ namespace shell { namespace sessioninstall
     {
         public:
             SyncDbusSessionHelper(css::uno::Reference< css::uno::XComponentContext> const&);
-            virtual ~SyncDbusSessionHelper() override {}
 
             // XModify Methods
             virtual void SAL_CALL InstallPackageFiles( ::sal_uInt32 xid, const css::uno::Sequence< OUString >& files, const OUString& interaction ) override;
diff --git a/xmlscript/source/xmldlg_imexp/imp_share.hxx b/xmlscript/source/xmldlg_imexp/imp_share.hxx
index 18598891bce7..c37174b70bc2 100644
--- a/xmlscript/source/xmldlg_imexp/imp_share.hxx
+++ b/xmlscript/source/xmldlg_imexp/imp_share.hxx
@@ -480,9 +480,6 @@ public:
                 xProps,
                 rControlName )
         {}
-    inline ~ControlImportContext()
-    {
-    }
 
     /// @throws css::xml::sax::SAXException
     /// @throws css::uno::RuntimeException