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>

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

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

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