From a6309a3c4db0f5b409d47b3af094952fd95d4be7 Mon Sep 17 00:00:00 2001 From: Stephan Bergmann Date: Tue, 7 Nov 2017 16:15:03 +0100 Subject: [PATCH] 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 Reviewed-by: Stephan Bergmann --- .clang-format | 1 + .../clang/test/unnecessaryoverride.cxx | 48 +++++++++++++++ compilerplugins/clang/unnecessaryoverride.cxx | 59 ++++++++++++++++--- solenv/CompilerTest_compilerplugins_clang.mk | 1 + 4 files changed, 102 insertions(+), 7 deletions(-) create mode 100644 compilerplugins/clang/test/unnecessaryoverride.cxx 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, 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 \