zinovy.nis updated this revision to Diff 137781.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D44295
Files:
clang-tidy/bugprone/BugproneTidyModule.cpp
clang-tidy/bugprone/CMakeLists.txt
clang-tidy/bugprone/ParentVirtualCallCheck.cpp
clang-tidy/bugprone/ParentVirtualCallCheck.h
docs/ReleaseNotes.rst
docs/clang-tidy/checks/bugprone-parent-virtual-call.rst
docs/clang-tidy/checks/list.rst
test/clang-tidy/bugprone-parent-virtual-call.cpp
Index: test/clang-tidy/bugprone-parent-virtual-call.cpp
===================================================================
--- /dev/null
+++ test/clang-tidy/bugprone-parent-virtual-call.cpp
@@ -0,0 +1,116 @@
+// RUN: %check_clang_tidy %s bugprone-parent-virtual-call %t
+
+extern int f();
+
+class A {
+public:
+ A() = default;
+ virtual ~A() = default;
+
+ virtual int virt_1() { return f() + 1; }
+ virtual int virt_2() { return f() + 2; }
+
+ int non_virt() { return f() + 3; }
+ static int stat() { return f() + 4; }
+};
+
+class B : public A {
+public:
+ B() = default;
+
+ // Nothing to fix: calls to parent.
+ int virt_1() override { return A::virt_1() + 3; }
+ int virt_2() override { return A::virt_2() + 4; }
+};
+
+class C : public B {
+public:
+ int virt_1() override { return A::virt_1() + B::virt_1(); }
+ // CHECK-MESSAGES: :[[@LINE-1]]:34: warning: 'A::virt_1' is a grand-parent's method, not parent's. Did you mean 'B'?
+ // CHECK-FIXES: int virt_1() override { return B::virt_1() + B::virt_1(); }
+ int virt_2() override { return A::virt_1() + B::virt_1(); }
+ // CHECK-MESSAGES: :[[@LINE-1]]:34: warning: 'A::virt_1' is a grand-parent's method, not parent's. Did you mean 'B'?
+ // CHECK-FIXES: int virt_2() override { return B::virt_1() + B::virt_1(); }
+
+ // Test that non-virtual and static methods are not affected by this cherker.
+ int method_c() { return A::stat() + A::non_virt(); }
+};
+
+// Test that the check affects grand-grand..-parent calls too.
+class D : public C {
+public:
+ int virt_1() override { return A::virt_1() + B::virt_1() + D::virt_1(); }
+ // CHECK-MESSAGES: :[[@LINE-1]]:34: warning: 'A::virt_1' is a grand-parent's method, not parent's. Did you mean 'C'?
+ // CHECK-MESSAGES: :[[@LINE-2]]:48: warning: 'B::virt_1' is a grand-parent's method, not parent's. Did you mean 'C'?
+ // CHECK-FIXES: int virt_1() override { return C::virt_1() + C::virt_1() + D::virt_1(); }
+ int virt_2() override { return A::virt_1() + B::virt_1() + D::virt_1(); }
+ // CHECK-MESSAGES: :[[@LINE-1]]:34: warning: 'A::virt_1' is a grand-parent's method, not parent's. Did you mean 'C'?
+ // CHECK-MESSAGES: :[[@LINE-2]]:48: warning: 'B::virt_1' is a grand-parent's method, not parent's. Did you mean 'C'?
+ // CHECK-FIXES: int virt_2() override { return C::virt_1() + C::virt_1() + D::virt_1(); }
+};
+
+// Test classes in anonymous namespaces.
+namespace {
+class BN : public A {
+public:
+ int virt_1() override { return A::virt_1() + 3; }
+ int virt_2() override { return A::virt_2() + 4; }
+};
+} // namespace N
+
+class CN : public BN {
+public:
+ int virt_1() override { return A::virt_1() + BN::virt_1(); }
+ // CHECK-MESSAGES: :[[@LINE-1]]:34: warning: 'A::virt_1' is a grand-parent's method, not parent's. Did you mean '(anonymous namespace)::BN'?
+ // CHECK-FIXES: int virt_1() override { return BN::virt_1() + BN::virt_1(); }
+ int virt_2() override { return A::virt_1() + BN::virt_1(); }
+ // CHECK-MESSAGES: :[[@LINE-1]]:34: warning: 'A::virt_1' is a grand-parent's method, not parent's. Did you mean '(anonymous namespace)::BN'?
+ // CHECK-FIXES: int virt_2() override { return BN::virt_1() + BN::virt_1(); }
+};
+
+// Test multiple inheritance fixes
+class AA {
+public:
+ AA() = default;
+ virtual ~AA() = default;
+
+ virtual int virt_1() { return f() + 1; }
+ virtual int virt_2() { return f() + 2; }
+
+ int non_virt() { return f() + 3; }
+ static int stat() { return f() + 4; }
+};
+
+class BB_1 : virtual public AA {
+public:
+ BB_1() = default;
+
+ // Nothing to fix: calls to parent.
+ int virt_1() override { return AA::virt_1() + 3; }
+ int virt_2() override { return AA::virt_2() + 4; }
+};
+
+class BB_2 : virtual public AA {
+public:
+ BB_2() = default;
+};
+
+class CC : public BB_1, public BB_2 {
+public:
+ int virt_1() override { return AA::virt_1() + 3; }
+ // CHECK-MESSAGES: :[[@LINE-1]]:34: warning: 'AA::virt_1' is a grand-parent's method, not parent's. Did you mean 'BB_1' or 'BB_2'? [bugprone-parent-virtual-call]
+ // No fix available due to multiple choise of parent class.
+};
+
+// Test templated classes.
+template <class F> class BF : public A {
+public:
+ int virt_1() override { return A::virt_1() + 3; }
+};
+
+class CF : public BF<int> {
+public:
+ int virt_1() override { return A::virt_1(); }
+ // CHECK-MESSAGES: :[[@LINE-1]]:34: warning: 'A::virt_1' is a grand-parent's method, not parent's. Did you mean 'BF'?
+ // CHECK-FIXES: int virt_1() override { return BF::virt_1(); }
+};
Index: docs/clang-tidy/checks/list.rst
===================================================================
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -34,6 +34,7 @@
bugprone-misplaced-widening-cast
bugprone-move-forwarding-reference
bugprone-multiple-statement-macro
+ bugprone-parent-virtual-call
bugprone-string-constructor
bugprone-string-integer-assignment
bugprone-string-literal-with-embedded-nul
Index: docs/clang-tidy/checks/bugprone-parent-virtual-call.rst
===================================================================
--- /dev/null
+++ docs/clang-tidy/checks/bugprone-parent-virtual-call.rst
@@ -0,0 +1,26 @@
+.. title:: clang-tidy - bugprone-parent-virtual-call
+
+bugprone-parent-virtual-call
+============================
+
+Detects and fixes calls to grand-...parent virtual methods instead of calls to parent's virtual methods.
+For example:
+
+class A {
+...
+int virtual foo() {...}
+...
+}
+
+class B: public A {
+...
+int foo() override {...}
+...
+}
+
+class C: public B {
+...
+int foo() override {... A::foo()...}
+// ^^^^^^^^ warning: 'A::foo' is a grand-parent's method, not parent's. Did you mean 'B'? [bugprone-parent-virtual-call]
+...
+}
Index: docs/ReleaseNotes.rst
===================================================================
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -70,6 +70,13 @@
with looping constructs. Every backward jump is rejected. Forward jumps are
only allowed in nested loops.
+- New `bugprone-parent-virtual-call
+ <http://clang.llvm.org/extra/clang-tidy/checks/bugprone-parent-virtual-call.html>`_ check
+
+ Warns if one calls grand-..parent's virtual method in child's virtual
+ method instead of parent's. Can automatically fix such cases by retargeting
+ calls to parent.
+
- New `fuchsia-multiple-inheritance
<http://clang.llvm.org/extra/clang-tidy/checks/fuchsia-multiple-inheritance.html>`_ check
Index: clang-tidy/bugprone/ParentVirtualCallCheck.h
===================================================================
--- /dev/null
+++ clang-tidy/bugprone/ParentVirtualCallCheck.h
@@ -0,0 +1,35 @@
+//===--- ParentVirtualCallCheck.h - clang-tidy-------------------*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_PARENTVIRTUALCALLCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_PARENTVIRTUALCALLCHECK_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace bugprone {
+
+/// Finds calls to grand..-parent virtual methods instead of parent's.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/bugprone-parent-virtual-call.html
+class ParentVirtualCallCheck : public ClangTidyCheck {
+public:
+ ParentVirtualCallCheck(StringRef Name, ClangTidyContext *Context)
+ : ClangTidyCheck(Name, Context) {}
+ void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+ void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace bugprone
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_PARENTVIRTUALCALLCHECK_H
Index: clang-tidy/bugprone/ParentVirtualCallCheck.cpp
===================================================================
--- /dev/null
+++ clang-tidy/bugprone/ParentVirtualCallCheck.cpp
@@ -0,0 +1,158 @@
+//===--- ParentVirtualCallCheck.cpp - clang-tidy---------------------------===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "ParentVirtualCallCheck.h"
+
+#include <algorithm>
+#include <list>
+
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace bugprone {
+
+namespace {
+
+bool IsParentOf(const CXXRecordDecl *Parent, const CXXRecordDecl *ThisClass) {
+ assert(Parent);
+ assert(ThisClass);
+ if (Parent->getCanonicalDecl() == ThisClass->getCanonicalDecl())
+ return true;
+ const auto ClassIter = std::find_if(
+ ThisClass->bases_begin(), ThisClass->bases_end(), [=](auto &Base) {
+ assert(Base.getType()->getAsCXXRecordDecl());
+ return Parent->getCanonicalDecl() ==
+ Base.getType()->getAsCXXRecordDecl()->getCanonicalDecl();
+ });
+ return ClassIter != ThisClass->bases_end();
+}
+
+bool IsRecursiveParentOf(const CXXRecordDecl *Parent,
+ const CXXRecordDecl *ThisClass) {
+ assert(Parent);
+ assert(ThisClass);
+ return ThisClass->isDerivedFrom(Parent);
+}
+
+std::list<const CXXRecordDecl *>
+GetParentsByGrandParent(const CXXRecordDecl *GrandParent,
+ const CXXRecordDecl *ThisClass) {
+
+ assert(GrandParent != nullptr);
+ assert(ThisClass != nullptr);
+ std::list<const CXXRecordDecl *> result;
+ for (auto &Base : ThisClass->bases()) {
+ if (IsRecursiveParentOf(
+ GrandParent,
+ Base.getType()->getAsCXXRecordDecl()->getCanonicalDecl()))
+ result.push_back(
+ Base.getType()->getAsCXXRecordDecl()->getCanonicalDecl());
+ }
+ return result;
+}
+
+// The same as NamedDecl::getQualifiedNameAsString, but with custom
+// PrintingPolicy for anonymous namespaces.
+std::string GetNameAsString(const NamedDecl &Decl) {
+ PrintingPolicy PP(Decl.getASTContext().getPrintingPolicy());
+ PP.SuppressUnwrittenScope = true;
+ std::string QualName;
+ llvm::raw_string_ostream OS(QualName);
+ Decl.printQualifiedName(OS, PP);
+ return OS.str();
+}
+} // namespace
+
+void ParentVirtualCallCheck::registerMatchers(MatchFinder *Finder) {
+ Finder->addMatcher(
+ cxxMemberCallExpr(
+ callee(memberExpr(hasDescendant(implicitCastExpr(
+ hasImplicitDestinationType(pointsTo(
+ type(anything()).bind("castToType"))),
+ hasSourceExpression(cxxThisExpr(hasType(
+ type(anything()).bind("thisType")))))))
+ .bind("member")),
+ callee(cxxMethodDecl(isVirtual())))
+ .bind("call"),
+ this);
+}
+
+void ParentVirtualCallCheck::check(const MatchFinder::MatchResult &Result) {
+
+ const auto *MatchedDecl = Result.Nodes.getNodeAs<CXXMemberCallExpr>("call");
+
+ assert(MatchedDecl);
+ assert(MatchedDecl->getExprLoc().isValid());
+
+ auto *Member = Result.Nodes.getNodeAs<MemberExpr>("member");
+ assert(Member);
+
+ auto *ThisTypePtr = Result.Nodes.getNodeAs<PointerType>("thisType");
+ assert(ThisTypePtr);
+
+ auto *ThisType = ThisTypePtr->getPointeeCXXRecordDecl();
+ assert(ThisType);
+
+ auto *CastToTypePtr = Result.Nodes.getNodeAs<RecordType>("castToType");
+ clang::CXXRecordDecl *CastToType = nullptr;
+
+ if (CastToTypePtr)
+ CastToType = CastToTypePtr->getAsCXXRecordDecl();
+ else {
+ if (auto *TemplateSpecializationTypePtr =
+ Result.Nodes.getNodeAs<TemplateSpecializationType>("castToType")) {
+ CastToType = TemplateSpecializationTypePtr->getAsCXXRecordDecl();
+ }
+ }
+ assert(CastToType);
+
+ if (IsParentOf(CastToType, ThisType)) {
+ return;
+ }
+
+ const auto Parents = GetParentsByGrandParent(CastToType, ThisType);
+ assert(!Parents.empty());
+
+ auto ParentIter = Parents.begin();
+ std::string ParentsStr =
+ "'" + (*ParentIter)->getQualifiedNameAsString() + "'";
+ for (++ParentIter; ParentIter != Parents.end(); ++ParentIter)
+ ParentsStr += " or '" + (*ParentIter)->getQualifiedNameAsString() + "'";
+
+ const auto *qual = Member->getQualifier();
+ if (!qual)
+ return;
+
+ std::string CalleeName;
+ {
+ llvm::raw_string_ostream OStream(CalleeName);
+ MatchedDecl->getCalleeDecl()->getAsFunction()->printQualifiedName(OStream);
+ }
+
+ assert(Member->getQualifierLoc().getSourceRange().getBegin().isValid());
+ auto Diag =
+ diag(Member->getQualifierLoc().getSourceRange().getBegin(),
+ "'%0' is a grand-parent's method, not parent's. Did you mean %1?")
+ << CalleeName << ParentsStr;
+
+ if (Parents.size() == 1 &&
+ Member->getQualifierLoc().getSourceRange().getBegin().isValid()) {
+ Diag << FixItHint::CreateReplacement(
+ Member->getQualifierLoc().getSourceRange(),
+ GetNameAsString(*(Parents.front())) + "::");
+ }
+}
+
+} // namespace bugprone
+} // namespace tidy
+} // namespace clang
Index: clang-tidy/bugprone/CMakeLists.txt
===================================================================
--- clang-tidy/bugprone/CMakeLists.txt
+++ clang-tidy/bugprone/CMakeLists.txt
@@ -19,6 +19,7 @@
MisplacedWideningCastCheck.cpp
MoveForwardingReferenceCheck.cpp
MultipleStatementMacroCheck.cpp
+ ParentVirtualCallCheck.cpp
StringConstructorCheck.cpp
StringIntegerAssignmentCheck.cpp
StringLiteralWithEmbeddedNulCheck.cpp
Index: clang-tidy/bugprone/BugproneTidyModule.cpp
===================================================================
--- clang-tidy/bugprone/BugproneTidyModule.cpp
+++ clang-tidy/bugprone/BugproneTidyModule.cpp
@@ -27,6 +27,7 @@
#include "MisplacedWideningCastCheck.h"
#include "MoveForwardingReferenceCheck.h"
#include "MultipleStatementMacroCheck.h"
+#include "ParentVirtualCallCheck.h"
#include "StringConstructorCheck.h"
#include "StringIntegerAssignmentCheck.h"
#include "StringLiteralWithEmbeddedNulCheck.h"
@@ -83,6 +84,8 @@
"bugprone-move-forwarding-reference");
CheckFactories.registerCheck<MultipleStatementMacroCheck>(
"bugprone-multiple-statement-macro");
+ CheckFactories.registerCheck<ParentVirtualCallCheck>(
+ "bugprone-parent-virtual-call");
CheckFactories.registerCheck<StringConstructorCheck>(
"bugprone-string-constructor");
CheckFactories.registerCheck<StringIntegerAssignmentCheck>(
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
