[Libreoffice-commits] core.git: .clang-format compilerplugins/clang solenv/CompilerTest_compilerplugins_clang.mk
Stephan Bergmann
sbergman at redhat.com
Wed Nov 8 06:53:59 UTC 2017
.clang-format | 1
compilerplugins/clang/test/unnecessaryoverride.cxx | 48 +++++++++++++++++
compilerplugins/clang/unnecessaryoverride.cxx | 59 ++++++++++++++++++---
solenv/CompilerTest_compilerplugins_clang.mk | 1
4 files changed, 102 insertions(+), 7 deletions(-)
New commits:
commit a6309a3c4db0f5b409d47b3af094952fd95d4be7
Author: Stephan Bergmann <sbergman at redhat.com>
Date: Tue Nov 7 16:15:03 2017 +0100
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 at libreoffice.org>
Reviewed-by: Stephan Bergmann <sbergman at redhat.com>
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 \
More information about the Libreoffice-commits
mailing list