Suppress loplugin:unnecessaryoverride...

...when a class derives from multiple (non-virtual) instances of one base class,
and the override disambiguates which of those instances' member to call.

That was the case with SwXTextDocument::queryAdapter
(sw/source/uibase/uno/unotxdoc.cxx), where SwXTextDocument derives from
cppu::OWeakObject through both SwXTextDocumentBaseClass and SfxBaseModel, but
calling queryAdapter through a pointer to SwXTextDocumentBaseClass apparently
needs to call OWeakObject::queryAdapter on the second, SfxBaseModel-inherited
OWeakObject base instance, or else CppunitTest_sw_macros_test fails.

Who knows what other instances of similar non-unnecessary overrides have been
removed with the help of broken loplugin:unnecessaryoverride, for which there
were no tests that started to fail...

Turns out .clang-format lacked "ReflowComments: false" to not break the special
"// expected-error {{...}}" etc. comments in compilerplugins/clang/test/.

Also, use a better location to report loplugin:unnecessaryoverride, to keep
clang-format and loplugin:unnecessaryoverride from fighting over how to split
lines and where to put the comment in
compilerplugins/clang/test/unnecessaryoverride.cxx.

Change-Id: I3b24df24369db12f8ec1080d6c9f7b70ff561a16
Reviewed-on: https://gerrit.libreoffice.org/44418
Tested-by: Jenkins <ci@libreoffice.org>
Reviewed-by: Stephan Bergmann <sbergman@redhat.com>

4 files changed, 102 insertions, 7 deletions

diff --git a/.clang-format b/.clang-format
index 10c6fd8482cd..338e8d627121 100644
--- a/.clang-format
+++ b/.clang-format
@@ -26,6 +26,7 @@ PenaltyBreakFirstLessLess: 120
 PenaltyExcessCharacter: 1000000
 PenaltyReturnTypeOnItsOwnLine: 60
 PointerBindsToType: true
+ReflowComments: false
 SpacesBeforeTrailingComments: 1
 Cpp11BracedListStyle: false
 Standard:        Cpp11
diff --git a/compilerplugins/clang/test/unnecessaryoverride.cxx b/compilerplugins/clang/test/unnecessaryoverride.cxx
new file mode 100644
index 000000000000..f50869e542ee
--- /dev/null
+++ b/compilerplugins/clang/test/unnecessaryoverride.cxx
@@ -0,0 +1,48 @@
+/* -*- 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/.
+ */
+
+struct Base
+{
+    virtual ~Base();
+    virtual void f();
+};
+
+struct SimpleDerived : Base
+{
+    void
+    f() override // expected-error {{public virtual function just calls public parent [loplugin:unnecessaryoverride]}}
+    {
+        Base::f();
+    }
+};
+
+struct Intermediate1 : Base
+{
+};
+
+struct MultiFunctionIntermediate2 : Base
+{
+    void f() override;
+};
+
+struct MultiFunctionDerived : Intermediate1, MultiFunctionIntermediate2
+{
+    void f() override { Intermediate1::f(); } // no warning
+};
+
+struct MultiClassIntermediate2 : Base
+{
+};
+
+struct MultiClassDerived : Intermediate1, MultiClassIntermediate2
+{
+    void f() override { Intermediate1::f(); } // no warning
+};
+
+/* 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 b54c821e0fe0..b71cf2003f3e 100644
--- a/compilerplugins/clang/unnecessaryoverride.cxx
+++ b/compilerplugins/clang/unnecessaryoverride.cxx
@@ -23,6 +23,47 @@ look for methods where all they do is call their superclass method
 
 namespace {
 
+bool hasMultipleBaseInstances_(
+    CXXRecordDecl const * derived, CXXRecordDecl const * canonicBase,
+    bool & hasAsNonVirtualBase, bool & hasAsVirtualBase)
+{
+    for (auto i = derived->bases_begin(); i != derived->bases_end(); ++i) {
+        auto const cls = i->getType()->getAsCXXRecordDecl();
+        if (cls == nullptr) {
+            assert(i->getType()->isDependentType());
+            // Conservatively assume "yes" for dependent bases:
+            return true;
+        }
+        if (cls->getCanonicalDecl() == canonicBase) {
+            if (i->isVirtual()) {
+                if (hasAsNonVirtualBase) {
+                    return true;
+                }
+                hasAsVirtualBase = true;
+            } else {
+                if (hasAsNonVirtualBase || hasAsVirtualBase) {
+                    return true;
+                }
+                hasAsNonVirtualBase = true;
+            }
+        } else if (hasMultipleBaseInstances_(
+                       cls, canonicBase, hasAsNonVirtualBase, hasAsVirtualBase))
+        {
+            return true;
+        }
+    }
+    return false;
+}
+
+bool hasMultipleBaseInstances(
+    CXXRecordDecl const * derived, CXXRecordDecl const * base)
+{
+    bool nonVirt = false;
+    bool virt = false;
+    return hasMultipleBaseInstances_(
+        derived, base->getCanonicalDecl(), nonVirt, virt);
+}
+
 class UnnecessaryOverride:
     public RecursiveASTVisitor<UnnecessaryOverride>, public loplugin::Plugin
 {
@@ -202,12 +243,20 @@ bool UnnecessaryOverride::VisitCXXMethodDecl(const CXXMethodDecl* methodDecl)
         return true;
     }
 
-    // if we are overriding more than one method, then this is a disambiguating override
+    // If overriding more than one base member function, or one base member
+    // function that is available in multiple (non-virtual) base class
+    // instances, then this is a disambiguating override:
     if (methodDecl->isVirtual()) {
         if (methodDecl->size_overridden_methods() != 1)
         {
             return true;
         }
+        if (hasMultipleBaseInstances(
+                methodDecl->getParent(),
+                (*methodDecl->begin_overridden_methods())->getParent()))
+        {
+            return true;
+        }
     }
     // sometimes the disambiguation happens in a base class
     if (loplugin::isSamePathname(aFileName, SRCDIR "/comphelper/source/property/propertycontainer.cxx"))
@@ -221,10 +270,6 @@ bool UnnecessaryOverride::VisitCXXMethodDecl(const CXXMethodDecl* methodDecl)
     // entertaining template magic
     if (loplugin::isSamePathname(aFileName, SRCDIR "/sc/source/ui/vba/vbaformatcondition.cxx"))
         return true;
-    // not sure what is going on here, but removing the override causes a crash
-     if (methodDecl->getQualifiedNameAsString() == "SwXTextDocument::queryAdapter")
-         return true;
-
 
     const CXXMethodDecl* overriddenMethodDecl = findOverriddenOrSimilarMethodInSuperclasses(methodDecl);
     if (!overriddenMethodDecl) {
@@ -317,7 +362,7 @@ bool UnnecessaryOverride::VisitCXXMethodDecl(const CXXMethodDecl* methodDecl)
 
     report(
             DiagnosticsEngine::Warning, "%0%1 function just calls %2 parent",
-            methodDecl->getSourceRange().getBegin())
+            methodDecl->getLocation())
         << methodDecl->getAccess()
         << (methodDecl->isVirtual() ? " virtual" : "")
         << overriddenMethodDecl->getAccess()
@@ -327,7 +372,7 @@ bool UnnecessaryOverride::VisitCXXMethodDecl(const CXXMethodDecl* methodDecl)
         report(
             DiagnosticsEngine::Note,
             "method declaration here",
-            pOther->getLocStart())
+            pOther->getLocation())
           << pOther->getSourceRange();
     }
     return true;
diff --git a/solenv/CompilerTest_compilerplugins_clang.mk b/solenv/CompilerTest_compilerplugins_clang.mk
index 4d9772fc4665..d68caf0b0720 100644
--- a/solenv/CompilerTest_compilerplugins_clang.mk
+++ b/solenv/CompilerTest_compilerplugins_clang.mk
@@ -41,6 +41,7 @@ $(eval $(call gb_CompilerTest_add_exception_objects,compilerplugins_clang, \
     compilerplugins/clang/test/salunicodeliteral \
     compilerplugins/clang/test/stringconstant \
     compilerplugins/clang/test/unnecessarycatchthrow \
+    compilerplugins/clang/test/unnecessaryoverride \
     compilerplugins/clang/test/unnecessaryoverride-dtor \
     compilerplugins/clang/test/unnecessaryparen \
     compilerplugins/clang/test/unoany \