[PATCH] D44295: [clang-tidy] Detects and fixes calls to grand-...parent virtual methods instead of calls to parent's virtual methods

[Zinovy Nis via Phabricator via cfe-commits]

zinovy.nis updated this revision to Diff 141199.
zinovy.nis added a comment.

- Switched to use 'tooling::fixit::getText' for qualified names.
- Shortened test code with {{.*}}.


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,179 @@
+// RUN: %check_clang_tidy %s bugprone-parent-virtual-call %t
+
+extern int foo();
+
+class A {
+public:
+  A() = default;
+  virtual ~A() = default;
+
+  virtual int virt_1() { return foo() + 1; }
+  virtual int virt_2() { return foo() + 2; }
+
+  int non_virt() { return foo() + 3; }
+  static int stat() { return foo() + 4; }
+};
+
+class B : public A {
+public:
+  B() = default;
+
+  // Nothing to fix: calls to direct 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: qualified name 'A::virt_1' refers to a member overridden in subclass; did you mean 'B'? [bugprone-parent-virtual-call]
+  // 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: qualified name 'A::virt_1' {{.*}}; 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(); }
+};
+
+// Check aliased type names
+using A1 = A;
+typedef A A2;
+#define A3 A
+
+class C2 : public B {
+public:
+  int virt_1() override { return A1::virt_1() + A2::virt_1() + A3::virt_1(); }
+  // CHECK-MESSAGES: :[[@LINE-1]]:34: warning: qualified name 'A1::virt_1' {{.*}}; did you mean 'B'? {{.*}}
+  // CHECK-MESSAGES: :[[@LINE-2]]:49: warning: qualified name 'A2::virt_1' {{.*}}; did you mean 'B'? {{.*}}
+  // CHECK-MESSAGES: :[[@LINE-3]]:64: warning: qualified name 'A3::virt_1' {{.*}}; did you mean 'B'? {{.*}}
+  // CHECK-FIXES:  int virt_1() override { return B::virt_1() + B::virt_1() + B::virt_1(); }
+};
+
+// 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: qualified name 'A::virt_1' {{.*}}; did you mean 'C'? {{.*}}
+  // CHECK-MESSAGES: :[[@LINE-2]]:48: warning: qualified name 'B::virt_1' {{.*}}; 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: qualified name 'A::virt_1' {{.*}}; did you mean 'C'? {{.*}}
+  // CHECK-MESSAGES: :[[@LINE-2]]:48: warning: qualified name 'B::virt_1' {{.*}}; did you mean 'C'? {{.*}}
+  // CHECK-FIXES:  int virt_2() override { return C::virt_1() + C::virt_1() + D::virt_1(); }
+};
+
+// Test classes in 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
+
+namespace N1 {
+class A {
+public:
+  A() = default;
+  virtual int virt_1() { return foo() + 1; }
+  virtual int virt_2() { return foo() + 2; }
+};
+} // namespace N1
+
+namespace N2 {
+class BN : public N1::A {
+public:
+  int virt_1() override { return A::virt_1() + 3; }
+  int virt_2() override { return A::virt_2() + 4; }
+};
+} // namespace N2
+
+class CN : public BN {
+public:
+  int virt_1() override { return A::virt_1() + BN::virt_1(); }
+  // CHECK-MESSAGES: :[[@LINE-1]]:34: warning: qualified name 'A::virt_1' {{.*}}; did you mean '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: qualified name 'A::virt_1' {{.*}}; did you mean 'BN'? {{.*}}
+  // CHECK-FIXES:  int virt_2() override { return BN::virt_1() + BN::virt_1(); }
+};
+
+class CNN : public N2::BN {
+public:
+  int virt_1() override { return N1::A::virt_1() + N2::BN::virt_1(); }
+  // CHECK-MESSAGES: :[[@LINE-1]]:34: warning: qualified name 'N1::A::virt_1' {{.*}}; did you mean 'N2::BN'? {{.*}}
+  // CHECK-FIXES:  int virt_1() override { return N2::BN::virt_1() + N2::BN::virt_1(); }
+  int virt_2() override { re

[PATCH] D44295: [clang-tidy] Detects and fixes calls to grand-...parent virtual methods instead of calls to parent's virtual methods

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh requested changes to this revision.
alexfh added inline comments.
This revision now requires changes to proceed.



Comment at: clang-tidy/bugprone/ParentVirtualCallCheck.cpp:73
+// 'typedef'ed grand-parent classes.
+// Adopted from https://stackoverflow.com/a/23081770/1073080
+static std::string getExprAsString(const clang::Expr &E,

That thread doesn't seem to be particularly useful. 
`Lexer::getSourceText(CharSourceRange::getTokenRange(E.getSourceRange()), SM, 
CorrectLangOpts, 0);` (or `clang::tooling::fixit::getText`) should work well in 
macro-free cases. When there are macros, `Lexer::makeFileCharRange` may help to 
find a contiguous range that corresponds to a `SourceRange` (and when it 
doesn't, this usually means that there's no such text range).

Both of these don't remove whitespace, but why would you do that anyway? If 
really needed, this can be done outside.



Comment at: clang-tidy/bugprone/ParentVirtualCallCheck.cpp:77
+  std::string Text = Lexer::getSourceText(
+  CharSourceRange::getTokenRange(E.getSourceRange()), SM, LangOptions(), 
0);
+  // Strip the string.

Any reason to not use the actual LangOptions? Substituting it with the default 
may result in incorrect handling of certain tokens.



Comment at: clang-tidy/bugprone/ParentVirtualCallCheck.cpp:79
+  // Strip the string.
+  Text.erase(std::remove_if(begin(Text), end(Text), std::isspace), end(Text));
+  if (!Text.empty() && (Text.at(Text.size() - 1) == ','))

Consider using `llvm::remove_if`.



Comment at: clang-tidy/bugprone/ParentVirtualCallCheck.cpp:80
+  Text.erase(std::remove_if(begin(Text), end(Text), std::isspace), end(Text));
+  if (!Text.empty() && (Text.at(Text.size() - 1) == ','))
+return Lexer::getSourceText(

Text.at(Text.size() - 1) seems to be the same as Text.back(). And what does 
this condition do, btw?



Comment at: clang-tidy/bugprone/ParentVirtualCallCheck.cpp:81-83
+return Lexer::getSourceText(
+CharSourceRange::getCharRange(E.getSourceRange()), SM, LangOptions(),
+0);

That seems to be a bad thing to fall back to - it uses getCharRange which will 
drop the last token.



Comment at: clang-tidy/bugprone/ParentVirtualCallCheck.cpp:144-146
+  // auto Diag = diag(MatchedDecl->getSourceRange().getBegin(), "variable
+  // declared here")
+  //<< MatchedDecl->getSourceRange();

Please remove the commented out code.



Comment at: test/clang-tidy/bugprone-parent-virtual-call.cpp:32
+  int virt_2() override { return A::virt_1() + B::virt_1(); }
+  // CHECK-MESSAGES: :[[@LINE-1]]:34: warning: qualified name 'A::virt_1' 
refers to a member overridden in subclass; did you mean 'B'? 
[bugprone-parent-virtual-call]
+  // CHECK-FIXES:  int virt_2() override { return B::virt_1() + B::virt_1(); }

alexfh wrote:
> Specifying each unique message completely once should be enough to detect 
> typos in it (e.g. missing or extra spaces or quotes around placeholders). In 
> other messages let's omit repeated static parts to make the test a bit more 
> easier to read, e.g.:
> 
>   // CHECK-MESSAGES: :[[@LINE-1]]:34: warning: qualified name 'A::virt_1' 
> {{.*}}; did you mean 'B'?
I'd also remove the ` [bugprone-parent-virtual-call]` part from repeated 
CHECK-patterns.


https://reviews.llvm.org/D44295



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44295: [clang-tidy] Detects and fixes calls to grand-...parent virtual methods instead of calls to parent's virtual methods

[Zinovy Nis via Phabricator via cfe-commits]

zinovy.nis updated this revision to Diff 140579.
zinovy.nis added a comment.

- Minor cosmetic fix: use lexical representation instead of semanic for 
printing callee string. It helps to print class names hidden behind 'typedef' 
and 'using':



  using A1 = A;
  typedef A A2;
  
  class C2 : public B {
  public:
int virt_1() override { return A1::virt_1() + A2::virt_1(); }
// BEFORE: warning: qualified name 'A::virt_1'...
// BEFORE: warning: qualified name 'A::virt_1'...
  
//  AFTER: warning: qualified name 'A1::virt_1'...
//  AFTER: warning: qualified name 'A2::virt_1'...
  };


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,177 @@
+// RUN: %check_clang_tidy %s bugprone-parent-virtual-call %t
+
+extern int foo();
+
+class A {
+public:
+  A() = default;
+  virtual ~A() = default;
+
+  virtual int virt_1() { return foo() + 1; }
+  virtual int virt_2() { return foo() + 2; }
+
+  int non_virt() { return foo() + 3; }
+  static int stat() { return foo() + 4; }
+};
+
+class B : public A {
+public:
+  B() = default;
+
+  // Nothing to fix: calls to direct 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: qualified name 'A::virt_1' refers to a member overridden in subclass; did you mean 'B'? [bugprone-parent-virtual-call]
+  // 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: qualified name 'A::virt_1' {{.*}}; did you mean 'B'? [bugprone-parent-virtual-call]
+  // 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(); }
+};
+
+// Check aliased type names
+using A1 = A;
+typedef A A2;
+
+class C2 : public B {
+public:
+  int virt_1() override { return A1::virt_1() + A2::virt_1(); }
+  // CHECK-MESSAGES: :[[@LINE-1]]:34: warning: qualified name 'A1::virt_1' {{.*}}; did you mean 'B'? [bugprone-parent-virtual-call]
+  // CHECK-MESSAGES: :[[@LINE-2]]:49: warning: qualified name 'A2::virt_1' {{.*}}; did you mean 'B'? [bugprone-parent-virtual-call]
+  // CHECK-FIXES:  int virt_1() override { return B::virt_1() + B::virt_1(); }
+};
+
+// 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: qualified name 'A::virt_1' {{.*}}; did you mean 'C'? [bugprone-parent-virtual-call]
+  // CHECK-MESSAGES: :[[@LINE-2]]:48: warning: qualified name 'B::virt_1' {{.*}}; did you mean 'C'? [bugprone-parent-virtual-call]
+  // 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: qualified name 'A::virt_1' {{.*}}; did you mean 'C'? [bugprone-parent-virtual-call]
+  // CHECK-MESSAGES: :[[@LINE-2]]:48: warning: qualified name 'B::virt_1' {{.*}}; did you mean 'C'? [bugprone-parent-virtual-call]
+  // CHECK-FIXES:  int virt_2() override { return C::virt_1() + C::virt_1() + D::virt_1(); }
+};
+
+// Test classes in 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
+
+namespace N1 {
+class A {
+public:
+  A() = default;
+  virtual int virt_1() { return foo() + 1; }
+  virtual int virt_2() { return foo() + 2; }
+};
+} // namespace N1
+
+namespace N2 {
+class BN : public N1::A {
+public:
+  int virt_1() override { return A::virt_1() + 3; }
+  int virt_2() override { return A::virt_2() + 4; }
+};
+} // namespace N2
+
+class CN : public BN {
+public:
+  int virt_1() override { return A::virt_1() + BN::virt_1(); }
+  // CHECK-MESSAGES: :[[@LINE-1]]:34: warning: qualified name 'A::virt_1' {{.*}}; did you mean 'BN'? [bugprone-parent-virtual-call]
+  // 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: qualified name 'A::virt_1'

[PATCH] D44295: [clang-tidy] Detects and fixes calls to grand-...parent virtual methods instead of calls to parent's virtual methods

[Zinovy Nis via Phabricator via cfe-commits]

zinovy.nis added a comment.

BTW, I've recently found few dozens of issues in the Chromium code with my 
check.
For ex.:

  browser/src/cc/layers/nine_patch_layer_impl.cc:89:51: warning: qualified name 
'cc::LayerImpl::LayerAsJson' refers to a member overridden in subclass; did you 
mean 'cc::UIResourceLayerImpl'? [bugprone-parent-virtual-call]
std::unique_ptr result = LayerImpl::LayerAsJson();
^~
cc::UIResourceLayerImpl::


https://reviews.llvm.org/D44295



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44295: [clang-tidy] Detects and fixes calls to grand-...parent virtual methods instead of calls to parent's virtual methods

[Zinovy Nis via Phabricator via cfe-commits]

zinovy.nis updated this revision to Diff 140475.
zinovy.nis marked 3 inline comments as done.
zinovy.nis added a comment.

- Cosmetic fixes.


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,177 @@
+// RUN: %check_clang_tidy %s bugprone-parent-virtual-call %t
+
+extern int foo();
+
+class A {
+public:
+  A() = default;
+  virtual ~A() = default;
+
+  virtual int virt_1() { return foo() + 1; }
+  virtual int virt_2() { return foo() + 2; }
+
+  int non_virt() { return foo() + 3; }
+  static int stat() { return foo() + 4; }
+};
+
+class B : public A {
+public:
+  B() = default;
+
+  // Nothing to fix: calls to direct 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: qualified name 'A::virt_1' refers to a member overridden in subclass; did you mean 'B'? [bugprone-parent-virtual-call]
+  // 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: qualified name 'A::virt_1' {{.*}}; did you mean 'B'? [bugprone-parent-virtual-call]
+  // 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(); }
+};
+
+// Check aliased type names
+using A1 = A;
+typedef A A2;
+
+class C2 : public B {
+public:
+  int virt_1() override { return A1::virt_1() + A2::virt_1(); }
+  // CHECK-MESSAGES: :[[@LINE-1]]:34: warning: qualified name 'A::virt_1' {{.*}}; did you mean 'B'? [bugprone-parent-virtual-call]
+  // CHECK-MESSAGES: :[[@LINE-2]]:49: warning: qualified name 'A::virt_1' {{.*}}; did you mean 'B'? [bugprone-parent-virtual-call]
+  // CHECK-FIXES:  int virt_1() override { return B::virt_1() + B::virt_1(); }
+};
+
+// 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: qualified name 'A::virt_1' {{.*}}; did you mean 'C'? [bugprone-parent-virtual-call]
+  // CHECK-MESSAGES: :[[@LINE-2]]:48: warning: qualified name 'B::virt_1' {{.*}}; did you mean 'C'? [bugprone-parent-virtual-call]
+  // 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: qualified name 'A::virt_1' {{.*}}; did you mean 'C'? [bugprone-parent-virtual-call]
+  // CHECK-MESSAGES: :[[@LINE-2]]:48: warning: qualified name 'B::virt_1' {{.*}}; did you mean 'C'? [bugprone-parent-virtual-call]
+  // CHECK-FIXES:  int virt_2() override { return C::virt_1() + C::virt_1() + D::virt_1(); }
+};
+
+// Test classes in 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
+
+namespace N1 {
+class A {
+public:
+  A() = default;
+  virtual int virt_1() { return foo() + 1; }
+  virtual int virt_2() { return foo() + 2; }
+};
+} // namespace N1
+
+namespace N2 {
+class BN : public N1::A {
+public:
+  int virt_1() override { return A::virt_1() + 3; }
+  int virt_2() override { return A::virt_2() + 4; }
+};
+} // namespace N2
+
+class CN : public BN {
+public:
+  int virt_1() override { return A::virt_1() + BN::virt_1(); }
+  // CHECK-MESSAGES: :[[@LINE-1]]:34: warning: qualified name 'A::virt_1' {{.*}}; did you mean 'BN'? [bugprone-parent-virtual-call]
+  // 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: qualified name 'A::virt_1' {{.*}}; did you mean 'BN'? [bugprone-parent-virtual-call]
+  // CHECK-FIXES:  int virt_2() override { return BN::virt_1() + BN::virt_1(); }
+};
+
+class CNN : public N2::BN {
+public:
+  int virt_1() override { return N1::A::virt_1() + N2::BN::virt_1(); }
+  // CHECK-MESSAGES: :[[@LINE-1]]:34: warning: qualified name 'N1::A::virt_1' {{.*}}; did you mean 'N2::BN'? [bugprone-parent-virtual-call]
+  // CHECK-FIXES:  int virt_1() override { return N2::BN::virt_1() + 

[PATCH] D44295: [clang-tidy] Detects and fixes calls to grand-...parent virtual methods instead of calls to parent's virtual methods

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh requested changes to this revision.
alexfh added a comment.
This revision now requires changes to proceed.

A few more nits.




Comment at: clang-tidy/bugprone/ParentVirtualCallCheck.cpp:114-117
+  auto ParentIter = Parents.begin();
+  std::string ParentsStr = "'" + getNameAsString(*ParentIter) + "'";
+  for (++ParentIter; ParentIter != Parents.end(); ++ParentIter)
+ParentsStr += " or '" + getNameAsString(*ParentIter) + "'";

I suggest using 1. range for loop; 2. std::string::append instead of 
operator+/operator+=. Not that performance is extremely important here, but 
it's also cleaner, IMO:

```
std::string ParentsStr = "'";
for (const CXXRecordDecl *Parent : Parents) {
  if (ParentsStr.size() > 1)
ParentsStr.append("' or '");
  ParentsStr.append(getNameAsString(Parent));
}
ParentsStr.append("'");
```



Comment at: clang-tidy/bugprone/ParentVirtualCallCheck.cpp:126
+   "in subclass%1; did you mean %2?")
+  << (MatchedDecl->getCalleeDecl()->getAsFunction())
+  << (Parents.size() > 1 ? "es" : "") << ParentsStr;

Please remove the parentheses.



Comment at: test/clang-tidy/bugprone-parent-virtual-call.cpp:32
+  int virt_2() override { return A::virt_1() + B::virt_1(); }
+  // CHECK-MESSAGES: :[[@LINE-1]]:34: warning: qualified name 'A::virt_1' 
refers to a member overridden in subclass; did you mean 'B'? 
[bugprone-parent-virtual-call]
+  // CHECK-FIXES:  int virt_2() override { return B::virt_1() + B::virt_1(); }

Specifying each unique message completely once should be enough to detect typos 
in it (e.g. missing or extra spaces or quotes around placeholders). In other 
messages let's omit repeated static parts to make the test a bit more easier to 
read, e.g.:

  // CHECK-MESSAGES: :[[@LINE-1]]:34: warning: qualified name 'A::virt_1' 
{{.*}}; did you mean 'B'?


https://reviews.llvm.org/D44295



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44295: [clang-tidy] Detects and fixes calls to grand-...parent virtual methods instead of calls to parent's virtual methods

[Zinovy Nis via Phabricator via cfe-commits]

zinovy.nis updated this revision to Diff 139834.
zinovy.nis added a comment.

- Updated warning message to



> warning: qualified name 'A::foo' refers to a member overridden in subclass; 
> did you mean 'B'? [bugprone-parent-virtual-call]




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,177 @@
+// RUN: %check_clang_tidy %s bugprone-parent-virtual-call %t
+
+extern int foo();
+
+class A {
+public:
+  A() = default;
+  virtual ~A() = default;
+
+  virtual int virt_1() { return foo() + 1; }
+  virtual int virt_2() { return foo() + 2; }
+
+  int non_virt() { return foo() + 3; }
+  static int stat() { return foo() + 4; }
+};
+
+class B : public A {
+public:
+  B() = default;
+
+  // Nothing to fix: calls to direct 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: qualified name 'A::virt_1' refers to a member overridden in subclass; did you mean 'B'? [bugprone-parent-virtual-call]
+  // 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: qualified name 'A::virt_1' refers to a member overridden in subclass; did you mean 'B'? [bugprone-parent-virtual-call]
+  // 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(); }
+};
+
+// Check aliased type names
+using A1 = A;
+typedef A A2;
+
+class C2 : public B {
+public:
+  int virt_1() override { return A1::virt_1() + A2::virt_1(); }
+  // CHECK-MESSAGES: :[[@LINE-1]]:34: warning: qualified name 'A::virt_1' refers to a member overridden in subclass; did you mean 'B'? [bugprone-parent-virtual-call]
+  // CHECK-MESSAGES: :[[@LINE-2]]:49: warning: qualified name 'A::virt_1' refers to a member overridden in subclass; did you mean 'B'? [bugprone-parent-virtual-call]
+  // CHECK-FIXES:  int virt_1() override { return B::virt_1() + B::virt_1(); }
+};
+
+// 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: qualified name 'A::virt_1' refers to a member overridden in subclass; did you mean 'C'? [bugprone-parent-virtual-call]
+  // CHECK-MESSAGES: :[[@LINE-2]]:48: warning: qualified name 'B::virt_1' refers to a member overridden in subclass; did you mean 'C'? [bugprone-parent-virtual-call]
+  // 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: qualified name 'A::virt_1' refers to a member overridden in subclass; did you mean 'C'? [bugprone-parent-virtual-call]
+  // CHECK-MESSAGES: :[[@LINE-2]]:48: warning: qualified name 'B::virt_1' refers to a member overridden in subclass; did you mean 'C'? [bugprone-parent-virtual-call]
+  // CHECK-FIXES:  int virt_2() override { return C::virt_1() + C::virt_1() + D::virt_1(); }
+};
+
+// Test classes in 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
+
+namespace N1 {
+class A {
+public:
+  A() = default;
+  virtual int virt_1() { return foo() + 1; }
+  virtual int virt_2() { return foo() + 2; }
+};
+} // namespace N1
+
+namespace N2 {
+class BN : public N1::A {
+public:
+  int virt_1() override { return A::virt_1() + 3; }
+  int virt_2() override { return A::virt_2() + 4; }
+};
+} // namespace N2
+
+class CN : public BN {
+public:
+  int virt_1() override { return A::virt_1() + BN::virt_1(); }
+  // CHECK-MESSAGES: :[[@LINE-1]]:34: warning: qualified name 'A::virt_1' refers to a member overridden in subclass; did you mean 'BN'? [bugprone-parent-virtual-call]
+  // 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: qualified name 'A::virt_1' refers to a member overridden in subclass; did you mean 'BN'? [bugprone-parent-virtual

[PATCH] D44295: [clang-tidy] Detects and fixes calls to grand-...parent virtual methods instead of calls to parent's virtual methods

[Zinovy Nis via Phabricator via cfe-commits]

zinovy.nis updated this revision to Diff 139805.
zinovy.nis retitled this revision from "[clang-tidy] Detect and fix calls to 
grand-...parent virtual methods instead of calls to parent's virtual methods" 
to "[clang-tidy] Detects and fixes calls to grand-...parent virtual methods 
instead of calls to parent's virtual methods".
zinovy.nis added a comment.

- Beautify bugprone-parent-virtual-call.rst


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,177 @@
+// RUN: %check_clang_tidy %s bugprone-parent-virtual-call %t
+
+extern int foo();
+
+class A {
+public:
+  A() = default;
+  virtual ~A() = default;
+
+  virtual int virt_1() { return foo() + 1; }
+  virtual int virt_2() { return foo() + 2; }
+
+  int non_virt() { return foo() + 3; }
+  static int stat() { return foo() + 4; }
+};
+
+class B : public A {
+public:
+  B() = default;
+
+  // Nothing to fix: calls to direct 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: qualified name 'A::virt_1' refers to a member which was overridden in subclass 'B' [bugprone-parent-virtual-call]
+  // 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: qualified name 'A::virt_1' refers to a member which was overridden in subclass 'B' [bugprone-parent-virtual-call]
+  // 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(); }
+};
+
+// Check aliased type names
+using A1 = A;
+typedef A A2;
+
+class C2 : public B {
+public:
+  int virt_1() override { return A1::virt_1() + A2::virt_1(); }
+  // CHECK-MESSAGES: :[[@LINE-1]]:34: warning: qualified name 'A::virt_1' refers to a member which was overridden in subclass 'B' [bugprone-parent-virtual-call]
+  // CHECK-MESSAGES: :[[@LINE-2]]:49: warning: qualified name 'A::virt_1' refers to a member which was overridden in subclass 'B' [bugprone-parent-virtual-call]
+  // CHECK-FIXES:  int virt_1() override { return B::virt_1() + B::virt_1(); }
+};
+
+// 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: qualified name 'A::virt_1' refers to a member which was overridden in subclass 'C' [bugprone-parent-virtual-call]
+  // CHECK-MESSAGES: :[[@LINE-2]]:48: warning: qualified name 'B::virt_1' refers to a member which was overridden in subclass 'C' [bugprone-parent-virtual-call]
+  // 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: qualified name 'A::virt_1' refers to a member which was overridden in subclass 'C' [bugprone-parent-virtual-call]
+  // CHECK-MESSAGES: :[[@LINE-2]]:48: warning: qualified name 'B::virt_1' refers to a member which was overridden in subclass 'C' [bugprone-parent-virtual-call]
+  // CHECK-FIXES:  int virt_2() override { return C::virt_1() + C::virt_1() + D::virt_1(); }
+};
+
+// Test classes in 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
+
+namespace N1 {
+class A {
+public:
+  A() = default;
+  virtual int virt_1() { return foo() + 1; }
+  virtual int virt_2() { return foo() + 2; }
+};
+} // namespace N1
+
+namespace N2 {
+class BN : public N1::A {
+public:
+  int virt_1() override { return A::virt_1() + 3; }
+  int virt_2() override { return A::virt_2() + 4; }
+};
+} // namespace N2
+
+class CN : public BN {
+public:
+  int virt_1() override { return A::virt_1() + BN::virt_1(); }
+  // CHECK-MESSAGES: :[[@LINE-1]]:34: warning: qualified name 'A::virt_1' refers to a member which was overridden in subclass 'BN' [bugprone-parent-virtual-call]
+  // 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: warnin

[PATCH] D44295: [clang-tidy] Detects and fixes calls to grand-...parent virtual methods instead of calls to parent's virtual methods

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh requested changes to this revision.
alexfh added inline comments.
This revision now requires changes to proceed.



Comment at: docs/clang-tidy/checks/bugprone-parent-virtual-call.rst:26
+  int foo() override {... A::foo()...}
+   warning: qualified name A::foo refers to a
+member which was overridden in 

alexfh wrote:
> The formatting looks weird. Could you move `warning: .*` to the start of the 
> next line and put the whole message on the same line?
Could you address the "and put the whole message on the same line" part as well?

Please also put `// ` before `warning:` and `...`s to make the example look 
more like syntactically valid code.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44295



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44295: [clang-tidy] Detects and fixes calls to grand-...parent virtual methods instead of calls to parent's virtual methods

[Zinovy Nis via Phabricator via cfe-commits]

zinovy.nis updated this revision to Diff 139646.
zinovy.nis added a comment.

- Rebased.
- Added 1 more test for namespaced base clases
- Fixed minor issues.


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,177 @@
+// RUN: %check_clang_tidy %s bugprone-parent-virtual-call %t
+
+extern int foo();
+
+class A {
+public:
+  A() = default;
+  virtual ~A() = default;
+
+  virtual int virt_1() { return foo() + 1; }
+  virtual int virt_2() { return foo() + 2; }
+
+  int non_virt() { return foo() + 3; }
+  static int stat() { return foo() + 4; }
+};
+
+class B : public A {
+public:
+  B() = default;
+
+  // Nothing to fix: calls to direct 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: qualified name 'A::virt_1' refers to a member which was overridden in subclass 'B' [bugprone-parent-virtual-call]
+  // 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: qualified name 'A::virt_1' refers to a member which was overridden in subclass 'B' [bugprone-parent-virtual-call]
+  // 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(); }
+};
+
+// Check aliased type names
+using A1 = A;
+typedef A A2;
+
+class C2 : public B {
+public:
+  int virt_1() override { return A1::virt_1() + A2::virt_1(); }
+  // CHECK-MESSAGES: :[[@LINE-1]]:34: warning: qualified name 'A::virt_1' refers to a member which was overridden in subclass 'B' [bugprone-parent-virtual-call]
+  // CHECK-MESSAGES: :[[@LINE-2]]:49: warning: qualified name 'A::virt_1' refers to a member which was overridden in subclass 'B' [bugprone-parent-virtual-call]
+  // CHECK-FIXES:  int virt_1() override { return B::virt_1() + B::virt_1(); }
+};
+
+// 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: qualified name 'A::virt_1' refers to a member which was overridden in subclass 'C' [bugprone-parent-virtual-call]
+  // CHECK-MESSAGES: :[[@LINE-2]]:48: warning: qualified name 'B::virt_1' refers to a member which was overridden in subclass 'C' [bugprone-parent-virtual-call]
+  // 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: qualified name 'A::virt_1' refers to a member which was overridden in subclass 'C' [bugprone-parent-virtual-call]
+  // CHECK-MESSAGES: :[[@LINE-2]]:48: warning: qualified name 'B::virt_1' refers to a member which was overridden in subclass 'C' [bugprone-parent-virtual-call]
+  // CHECK-FIXES:  int virt_2() override { return C::virt_1() + C::virt_1() + D::virt_1(); }
+};
+
+// Test classes in 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
+
+namespace N1 {
+class A {
+public:
+  A() = default;
+  virtual int virt_1() { return foo() + 1; }
+  virtual int virt_2() { return foo() + 2; }
+};
+} // namespace N1
+
+namespace N2 {
+class BN : public N1::A {
+public:
+  int virt_1() override { return A::virt_1() + 3; }
+  int virt_2() override { return A::virt_2() + 4; }
+};
+} // namespace N2
+
+class CN : public BN {
+public:
+  int virt_1() override { return A::virt_1() + BN::virt_1(); }
+  // CHECK-MESSAGES: :[[@LINE-1]]:34: warning: qualified name 'A::virt_1' refers to a member which was overridden in subclass 'BN' [bugprone-parent-virtual-call]
+  // 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: qualified name 'A::virt_1' refers to a member which was overridden in subclass 'BN' [bugprone-parent-virtual-call]
+  // CHECK-FIXES:  int virt_2() override { return BN::virt_1() + BN::virt_1(); }
+};
+
+

[PATCH] D44295: [clang-tidy] Detects and fixes calls to grand-...parent virtual methods instead of calls to parent's virtual methods

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added inline comments.



Comment at: docs/clang-tidy/checks/bugprone-parent-virtual-call.rst:26
+  int foo() override {... A::foo()...}
+   warning: qualified name A::foo refers to a
+member which was overridden in 

The formatting looks weird. Could you move `warning: .*` to the start of the 
next line and put the whole message on the same line?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44295



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44295: [clang-tidy] Detects and fixes calls to grand-...parent virtual methods instead of calls to parent's virtual methods

[Malcolm Parsons via Phabricator via cfe-commits]

malcolm.parsons added a comment.

Please add a test where the parent class is in a differently named namespace.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44295



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44295: [clang-tidy] Detects and fixes calls to grand-...parent virtual methods instead of calls to parent's virtual methods

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: docs/ReleaseNotes.rst:73
 
+- New `bugprone-parent-virtual-call
+  
`_
 check

Please rebase from trunk and use //:doc:// for link.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44295



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44295: [clang-tidy] Detects and fixes calls to grand-...parent virtual methods instead of calls to parent's virtual methods

[Zinovy Nis via Phabricator via cfe-commits]

zinovy.nis marked 9 inline comments as done.
zinovy.nis added inline comments.



Comment at: docs/clang-tidy/checks/bugprone-parent-virtual-call.rst:6
+
+Detects and fixes calls to grand-...parent virtual methods instead of calls
+to parent's virtual methods.

aaron.ballman wrote:
> This formulation is a bit odd because of the `grand-...parent` part. I would 
> reword to be more along the lines of: detects calls to qualified function 
> names that refer to a function that is not explicitly declared by a direct 
> base class.
>  that is not explicitly declared by a direct base class.

In fact, it detects calls to qualified function names

> that explicitly overridden in subclasses.




Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44295



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44295: [clang-tidy] Detects and fixes calls to grand-...parent virtual methods instead of calls to parent's virtual methods

[Zinovy Nis via Phabricator via cfe-commits]

zinovy.nis updated this revision to Diff 139499.
zinovy.nis marked an inline comment as done.
zinovy.nis added a comment.

camelCase last minute fix.


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,150 @@
+// RUN: %check_clang_tidy %s bugprone-parent-virtual-call %t
+
+extern int foo();
+
+class A {
+public:
+  A() = default;
+  virtual ~A() = default;
+
+  virtual int virt_1() { return foo() + 1; }
+  virtual int virt_2() { return foo() + 2; }
+
+  int non_virt() { return foo() + 3; }
+  static int stat() { return foo() + 4; }
+};
+
+class B : public A {
+public:
+  B() = default;
+
+  // Nothing to fix: calls to direct 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: qualified name 'A::virt_1' refers to a member which was overridden in subclass 'B' [bugprone-parent-virtual-call]
+  // 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: qualified name 'A::virt_1' refers to a member which was overridden in subclass 'B' [bugprone-parent-virtual-call]
+  // 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(); }
+};
+
+// Check aliased type names
+using A1 = A;
+typedef A A2;
+
+class C2 : public B {
+public:
+  int virt_1() override { return A1::virt_1() + A2::virt_1(); }
+  // CHECK-MESSAGES: :[[@LINE-1]]:34: warning: qualified name 'A::virt_1' refers to a member which was overridden in subclass 'B' [bugprone-parent-virtual-call]
+  // CHECK-MESSAGES: :[[@LINE-2]]:49: warning: qualified name 'A::virt_1' refers to a member which was overridden in subclass 'B' [bugprone-parent-virtual-call]
+  // CHECK-FIXES:  int virt_1() override { return B::virt_1() + B::virt_1(); }
+};
+
+// 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: qualified name 'A::virt_1' refers to a member which was overridden in subclass 'C' [bugprone-parent-virtual-call]
+  // CHECK-MESSAGES: :[[@LINE-2]]:48: warning: qualified name 'B::virt_1' refers to a member which was overridden in subclass 'C' [bugprone-parent-virtual-call]
+  // 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: qualified name 'A::virt_1' refers to a member which was overridden in subclass 'C' [bugprone-parent-virtual-call]
+  // CHECK-MESSAGES: :[[@LINE-2]]:48: warning: qualified name 'B::virt_1' refers to a member which was overridden in subclass 'C' [bugprone-parent-virtual-call]
+  // 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: qualified name 'A::virt_1' refers to a member which was overridden in subclass 'BN' [bugprone-parent-virtual-call]
+  // 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: qualified name 'A::virt_1' refers to a member which was overridden in subclass 'BN' [bugprone-parent-virtual-call]
+  // 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 foo() + 1; }
+  virtual int virt_2() { return foo() + 2; }
+
+  int non_virt() { return foo() + 3; }
+  static int stat() { return foo() + 4; }
+};
+
+class BB_1 : virtual public AA {
+public:
+  BB_1() = default

[PATCH] D44295: [clang-tidy] Detects and fixes calls to grand-...parent virtual methods instead of calls to parent's virtual methods

[Zinovy Nis via Phabricator via cfe-commits]

zinovy.nis updated this revision to Diff 139496.
zinovy.nis added a comment.

Aaron, I applied the changes you suggest.

I also found and fixed a new case when grandparent method is called via 
'typedef'ed or 'using' type. There's also a new test (class C2) for it.


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,150 @@
+// RUN: %check_clang_tidy %s bugprone-parent-virtual-call %t
+
+extern int foo();
+
+class A {
+public:
+  A() = default;
+  virtual ~A() = default;
+
+  virtual int virt_1() { return foo() + 1; }
+  virtual int virt_2() { return foo() + 2; }
+
+  int non_virt() { return foo() + 3; }
+  static int stat() { return foo() + 4; }
+};
+
+class B : public A {
+public:
+  B() = default;
+
+  // Nothing to fix: calls to direct 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: qualified name 'A::virt_1' refers to a member which was overridden in subclass 'B' [bugprone-parent-virtual-call]
+  // 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: qualified name 'A::virt_1' refers to a member which was overridden in subclass 'B' [bugprone-parent-virtual-call]
+  // 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(); }
+};
+
+// Check aliased type names
+using A1 = A;
+typedef A A2;
+
+class C2 : public B {
+public:
+  int virt_1() override { return A1::virt_1() + A2::virt_1(); }
+  // CHECK-MESSAGES: :[[@LINE-1]]:34: warning: qualified name 'A::virt_1' refers to a member which was overridden in subclass 'B' [bugprone-parent-virtual-call]
+  // CHECK-MESSAGES: :[[@LINE-2]]:49: warning: qualified name 'A::virt_1' refers to a member which was overridden in subclass 'B' [bugprone-parent-virtual-call]
+  // CHECK-FIXES:  int virt_1() override { return B::virt_1() + B::virt_1(); }
+};
+
+// 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: qualified name 'A::virt_1' refers to a member which was overridden in subclass 'C' [bugprone-parent-virtual-call]
+  // CHECK-MESSAGES: :[[@LINE-2]]:48: warning: qualified name 'B::virt_1' refers to a member which was overridden in subclass 'C' [bugprone-parent-virtual-call]
+  // 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: qualified name 'A::virt_1' refers to a member which was overridden in subclass 'C' [bugprone-parent-virtual-call]
+  // CHECK-MESSAGES: :[[@LINE-2]]:48: warning: qualified name 'B::virt_1' refers to a member which was overridden in subclass 'C' [bugprone-parent-virtual-call]
+  // 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: qualified name 'A::virt_1' refers to a member which was overridden in subclass 'BN' [bugprone-parent-virtual-call]
+  // 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: qualified name 'A::virt_1' refers to a member which was overridden in subclass 'BN' [bugprone-parent-virtual-call]
+  // 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 foo() + 1; }
+  virtual int virt_2() { return foo() + 2; }
+
+  int non_virt() { return foo() + 

[PATCH] D44295: [clang-tidy] Detects and fixes calls to grand-...parent virtual methods instead of calls to parent's virtual methods

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang-tidy/bugprone/ParentVirtualCallCheck.cpp:128
+  diag(Member->getQualifierLoc().getSourceRange().getBegin(),
+   "'%0' is a grand-parent's method, not parent's. Did you mean %1?")
+  << CalleeName << ParentsStr;

zinovy.nis wrote:
> aaron.ballman wrote:
> > zinovy.nis wrote:
> > > aaron.ballman wrote:
> > > > The diagnostic should not contain punctuation aside from the question 
> > > > mark.
> > > > 
> > > > Also, the diagnostic uses some novel terminology in "grandparent's 
> > > > method". How about: "qualified function name %0 refers to a function 
> > > > that is not defined by a direct base class; did you mean %1?"
> > > "is not defined by a direct base class" is not a correct proposal. Issues 
> > > that this check finds are all about calling a grand-parent's method 
> > > instead of parent's whether base class defines this method or not.
> > > 
> > > How about 
> > > 
> > > > Qualified function name %0 refers to a function not from a direct base 
> > > > class; did you mean %1?
> > > 
> > > ?
> > Not keen on "from", but we could use `"qualified function name %q0 refers 
> > to a function that is not explicitly declared by a direct base class; did 
> > you mean %1?"`
> Changed to  
> 
> > qualified name %0 refers to a member which was overridden in subclass%1"
> 
> 
Please use `%q0` rather than calling your custom qualified name function. The 
diagnostics engine will handle printing the qualified name appropriately for 
you.



Comment at: clang-tidy/bugprone/ParentVirtualCallCheck.cpp:39
+   const CXXMethodDecl &MemberDecl) {
+  BasesVector result;
+  for (auto &Base : ThisClass.bases()) {

Local variables should be capitalized.



Comment at: clang-tidy/bugprone/ParentVirtualCallCheck.cpp:40-41
+  BasesVector result;
+  for (auto &Base : ThisClass.bases()) {
+auto *BaseDecl = Base.getType()->getAsCXXRecordDecl();
+const auto *ActualMemberDecl =

Please add the `const` qualifier to both of these.



Comment at: clang-tidy/bugprone/ParentVirtualCallCheck.cpp:42-43
+auto *BaseDecl = Base.getType()->getAsCXXRecordDecl();
+const auto *ActualMemberDecl =
+MemberDecl.getCorrespondingMethodInClass(BaseDecl);
+if (!ActualMemberDecl)

Do not use `auto` here.



Comment at: clang-tidy/bugprone/ParentVirtualCallCheck.cpp:49-53
+auto *TypePtr =
+ActualMemberDecl->getThisType(ActualMemberDecl->getASTContext())
+.getTypePtr();
+assert(TypePtr);
+auto *Type = TypePtr->getPointeeCXXRecordDecl();

Do not use `auto` here as the type is not explicitly spelled out in the 
initialization. Also, please do not name the local variable `Type` as 1) it 
seem to be a declaration, and 2) `Type` is a type name.



Comment at: clang-tidy/bugprone/ParentVirtualCallCheck.cpp:54
+auto *Type = TypePtr->getPointeeCXXRecordDecl();
+assert(Type);
+if (Type->getCanonicalDecl()->isDerivedFrom(&GrandParent))

Please add a message to your asserts so that if they trigger, the user gets 
something more helpful as the diagnostic. Same elsewhere.



Comment at: clang-tidy/bugprone/ParentVirtualCallCheck.cpp:89
+
+  auto *Member = Result.Nodes.getNodeAs("member");
+  assert(Member);

Please put appropriate explicit qualifiers on all of the `auto` uses (here and 
elsewhere).



Comment at: clang-tidy/bugprone/ParentVirtualCallCheck.cpp:93
+  auto *MemberDecl = dyn_cast(Member->getMemberDecl());
+  assert(MemberDecl);
+

Instead of `dyn_cast` followed by `assert()`, you should just use `cast<>`, but 
only if you're certain you cannot get anything other than a `CXXMethodDecl` out 
of the expression.



Comment at: clang-tidy/bugprone/ParentVirtualCallCheck.cpp:98
+
+  auto *ThisType = ThisTypePtr->getPointeeCXXRecordDecl();
+  assert(ThisType);

Do not use `auto` here.



Comment at: docs/clang-tidy/checks/bugprone-parent-virtual-call.rst:6
+
+Detects and fixes calls to grand-...parent virtual methods instead of calls
+to parent's virtual methods.

This formulation is a bit odd because of the `grand-...parent` part. I would 
reword to be more along the lines of: detects calls to qualified function names 
that refer to a function that is not explicitly declared by a direct base class.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44295



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44295: [clang-tidy] Detects and fixes calls to grand-...parent virtual methods instead of calls to parent's virtual methods

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added inline comments.



Comment at: clang-tidy/bugprone/ParentVirtualCallCheck.cpp:24
+
+static bool IsParentOf(const CXXRecordDecl &Parent,
+   const CXXRecordDecl &ThisClass) {

http://llvm.org/docs/CodingStandards.html says: "Function names should be verb 
phrases (as they represent actions), and command-like function should be 
imperative. The name should be camel case, and start with a lower case letter 
(e.g. openFile() or isFoo())."



Comment at: clang-tidy/bugprone/ParentVirtualCallCheck.cpp:28
+return true;
+  const auto ClassIter = llvm::find_if(ThisClass.bases(), [=](auto &Base) {
+auto *BaseDecl = Base.getType()->getAsCXXRecordDecl();

Is `Parent` the only capture? I'd prefer it to be explicit instead of automatic.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44295



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44295: [clang-tidy] Detects and fixes calls to grand-...parent virtual methods instead of calls to parent's virtual methods

[Zinovy Nis via Phabricator via cfe-commits]

zinovy.nis added a comment.

Gentle ping)


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44295



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44295: [clang-tidy] Detects and fixes calls to grand-...parent virtual methods instead of calls to parent's virtual methods

[Zinovy Nis via Phabricator via cfe-commits]

zinovy.nis marked 2 inline comments as done.
zinovy.nis added a comment.

Hope I satisfied all your requests now.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44295



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44295: [clang-tidy] Detects and fixes calls to grand-...parent virtual methods instead of calls to parent's virtual methods

[Zinovy Nis via Phabricator via cfe-commits]

zinovy.nis marked 6 inline comments as done.
zinovy.nis added inline comments.



Comment at: clang-tidy/bugprone/ParentVirtualCallCheck.cpp:128
+  diag(Member->getQualifierLoc().getSourceRange().getBegin(),
+   "'%0' is a grand-parent's method, not parent's. Did you mean %1?")
+  << CalleeName << ParentsStr;

aaron.ballman wrote:
> zinovy.nis wrote:
> > aaron.ballman wrote:
> > > The diagnostic should not contain punctuation aside from the question 
> > > mark.
> > > 
> > > Also, the diagnostic uses some novel terminology in "grandparent's 
> > > method". How about: "qualified function name %0 refers to a function that 
> > > is not defined by a direct base class; did you mean %1?"
> > "is not defined by a direct base class" is not a correct proposal. Issues 
> > that this check finds are all about calling a grand-parent's method instead 
> > of parent's whether base class defines this method or not.
> > 
> > How about 
> > 
> > > Qualified function name %0 refers to a function not from a direct base 
> > > class; did you mean %1?
> > 
> > ?
> Not keen on "from", but we could use `"qualified function name %q0 refers to 
> a function that is not explicitly declared by a direct base class; did you 
> mean %1?"`
Changed to  

> qualified name %0 refers to a member which was overridden in subclass%1"




Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44295



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44295: [clang-tidy] Detects and fixes calls to grand-...parent virtual methods instead of calls to parent's virtual methods

[Zinovy Nis via Phabricator via cfe-commits]

zinovy.nis marked 5 inline comments as done.
zinovy.nis added inline comments.



Comment at: test/clang-tidy/bugprone-parent-virtual-call.cpp:113
+  int virt_1() override { return A::virt_1(); }
+  // CHECK-MESSAGES: :[[@LINE-1]]:34: warning: qualified function name 
A::virt_1 refers to a function not from a direct base class; did you mean 'BI'? 
[bugprone-parent-virtual-call]
+  // CHECK-FIXES:  int virt_1() override { return BI::virt_1(); }

aaron.ballman wrote:
> zinovy.nis wrote:
> > aaron.ballman wrote:
> > > zinovy.nis wrote:
> > > > aaron.ballman wrote:
> > > > > This seems like a false positive to me. Yes, the virtual function is 
> > > > > technically exposed in `BI`, but why is the programmer obligated to 
> > > > > call that one rather than the one from `A`, which is written in the 
> > > > > source?
> > > > IMHO it's a matter of safety. Today virt_1() is not overridden in BI, 
> > > > but tomorrow someone will implement BI::virt_1() and it will silently 
> > > > lead to bugs or whatever.
> > > If tomorrow someone implements `BI::virt_1()`, then the check will start 
> > > diagnosing at that point.
> > Correct, but anyway I don't think it's a problem.
> I'd prefer to see this changed to not diagnose. I don't relish the idea of 
> explaining why that code diagnoses as it stands.
Done. Now the check looks for a most recent class with the overridden method. 
Please review this change.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44295



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44295: [clang-tidy] Detects and fixes calls to grand-...parent virtual methods instead of calls to parent's virtual methods

[Zinovy Nis via Phabricator via cfe-commits]

zinovy.nis updated this revision to Diff 138858.
zinovy.nis edited the summary of this revision.

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,138 @@
+// RUN: %check_clang_tidy %s bugprone-parent-virtual-call %t
+
+extern int foo();
+
+class A {
+public:
+  A() = default;
+  virtual ~A() = default;
+
+  virtual int virt_1() { return foo() + 1; }
+  virtual int virt_2() { return foo() + 2; }
+
+  int non_virt() { return foo() + 3; }
+  static int stat() { return foo() + 4; }
+};
+
+class B : public A {
+public:
+  B() = default;
+
+  // Nothing to fix: calls to direct 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: qualified name A::virt_1 refers to a member which was overridden in subclass 'B' [bugprone-parent-virtual-call]
+  // 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: qualified name A::virt_1 refers to a member which was overridden in subclass 'B' [bugprone-parent-virtual-call]
+  // 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: qualified name A::virt_1 refers to a member which was overridden in subclass 'C' [bugprone-parent-virtual-call]
+  // CHECK-MESSAGES: :[[@LINE-2]]:48: warning: qualified name B::virt_1 refers to a member which was overridden in subclass 'C' [bugprone-parent-virtual-call]
+  // 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: qualified name A::virt_1 refers to a member which was overridden in subclass 'C' [bugprone-parent-virtual-call]
+  // CHECK-MESSAGES: :[[@LINE-2]]:48: warning: qualified name B::virt_1 refers to a member which was overridden in subclass 'C' [bugprone-parent-virtual-call]
+  // 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: qualified name A::virt_1 refers to a member which was overridden in subclass 'BN' [bugprone-parent-virtual-call]
+  // 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: qualified name A::virt_1 refers to a member which was overridden in subclass 'BN' [bugprone-parent-virtual-call]
+  // 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 foo() + 1; }
+  virtual int virt_2() { return foo() + 2; }
+
+  int non_virt() { return foo() + 3; }
+  static int stat() { return foo() + 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;
+  int virt_1() override { return AA::virt_1() + 3; }
+};
+
+class CC : public BB_1, public BB_2 {
+public:
+  int virt_1() override { return AA::virt_1() + 3; }
+  // CHECK-MESSAGES: :[[@LINE-1]]:34: warning: qualified name AA::virt_1 refers to a member which was overridden in subclasses 'BB_1' and 'BB_2' [bugprone-parent-virtual-call]
+  // No fix available due to multiple choice of parent class.
+};
+
+// Test 

[PATCH] D44295: [clang-tidy] Detects and fixes calls to grand-...parent virtual methods instead of calls to parent's virtual methods

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang-tidy/bugprone/ParentVirtualCallCheck.cpp:128
+  diag(Member->getQualifierLoc().getSourceRange().getBegin(),
+   "'%0' is a grand-parent's method, not parent's. Did you mean %1?")
+  << CalleeName << ParentsStr;

zinovy.nis wrote:
> aaron.ballman wrote:
> > The diagnostic should not contain punctuation aside from the question mark.
> > 
> > Also, the diagnostic uses some novel terminology in "grandparent's method". 
> > How about: "qualified function name %0 refers to a function that is not 
> > defined by a direct base class; did you mean %1?"
> "is not defined by a direct base class" is not a correct proposal. Issues 
> that this check finds are all about calling a grand-parent's method instead 
> of parent's whether base class defines this method or not.
> 
> How about 
> 
> > Qualified function name %0 refers to a function not from a direct base 
> > class; did you mean %1?
> 
> ?
Not keen on "from", but we could use `"qualified function name %q0 refers to a 
function that is not explicitly declared by a direct base class; did you mean 
%1?"`



Comment at: clang-tidy/bugprone/ParentVirtualCallCheck.cpp:107-108
+
+  std::string CalleeName =
+  getNameAsString(MatchedDecl->getCalleeDecl()->getAsFunction());
+  assert(Member->getQualifierLoc().getSourceRange().getBegin().isValid());

You can get rid of this and just pass in the `Decl *` directly while using `%q` 
in the format string.



Comment at: test/clang-tidy/bugprone-parent-virtual-call.cpp:113
+  int virt_1() override { return A::virt_1(); }
+  // CHECK-MESSAGES: :[[@LINE-1]]:34: warning: qualified function name 
A::virt_1 refers to a function not from a direct base class; did you mean 'BI'? 
[bugprone-parent-virtual-call]
+  // CHECK-FIXES:  int virt_1() override { return BI::virt_1(); }

zinovy.nis wrote:
> aaron.ballman wrote:
> > zinovy.nis wrote:
> > > aaron.ballman wrote:
> > > > This seems like a false positive to me. Yes, the virtual function is 
> > > > technically exposed in `BI`, but why is the programmer obligated to 
> > > > call that one rather than the one from `A`, which is written in the 
> > > > source?
> > > IMHO it's a matter of safety. Today virt_1() is not overridden in BI, but 
> > > tomorrow someone will implement BI::virt_1() and it will silently lead to 
> > > bugs or whatever.
> > If tomorrow someone implements `BI::virt_1()`, then the check will start 
> > diagnosing at that point.
> Correct, but anyway I don't think it's a problem.
I'd prefer to see this changed to not diagnose. I don't relish the idea of 
explaining why that code diagnoses as it stands.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44295



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44295: [clang-tidy] Detects and fixes calls to grand-...parent virtual methods instead of calls to parent's virtual methods

[Zinovy Nis via Phabricator via cfe-commits]

zinovy.nis added inline comments.



Comment at: test/clang-tidy/bugprone-parent-virtual-call.cpp:113
+  int virt_1() override { return A::virt_1(); }
+  // CHECK-MESSAGES: :[[@LINE-1]]:34: warning: qualified function name 
A::virt_1 refers to a function not from a direct base class; did you mean 'BI'? 
[bugprone-parent-virtual-call]
+  // CHECK-FIXES:  int virt_1() override { return BI::virt_1(); }

aaron.ballman wrote:
> zinovy.nis wrote:
> > aaron.ballman wrote:
> > > This seems like a false positive to me. Yes, the virtual function is 
> > > technically exposed in `BI`, but why is the programmer obligated to call 
> > > that one rather than the one from `A`, which is written in the source?
> > IMHO it's a matter of safety. Today virt_1() is not overridden in BI, but 
> > tomorrow someone will implement BI::virt_1() and it will silently lead to 
> > bugs or whatever.
> If tomorrow someone implements `BI::virt_1()`, then the check will start 
> diagnosing at that point.
Correct, but anyway I don't think it's a problem.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44295



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44295: [clang-tidy] Detects and fixes calls to grand-...parent virtual methods instead of calls to parent's virtual methods

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: test/clang-tidy/bugprone-parent-virtual-call.cpp:113
+  int virt_1() override { return A::virt_1(); }
+  // CHECK-MESSAGES: :[[@LINE-1]]:34: warning: qualified function name 
A::virt_1 refers to a function not from a direct base class; did you mean 'BI'? 
[bugprone-parent-virtual-call]
+  // CHECK-FIXES:  int virt_1() override { return BI::virt_1(); }

zinovy.nis wrote:
> aaron.ballman wrote:
> > This seems like a false positive to me. Yes, the virtual function is 
> > technically exposed in `BI`, but why is the programmer obligated to call 
> > that one rather than the one from `A`, which is written in the source?
> IMHO it's a matter of safety. Today virt_1() is not overridden in BI, but 
> tomorrow someone will implement BI::virt_1() and it will silently lead to 
> bugs or whatever.
If tomorrow someone implements `BI::virt_1()`, then the check will start 
diagnosing at that point.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44295



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44295: [clang-tidy] Detects and fixes calls to grand-...parent virtual methods instead of calls to parent's virtual methods

[Zinovy Nis via Phabricator via cfe-commits]

zinovy.nis added inline comments.



Comment at: test/clang-tidy/bugprone-parent-virtual-call.cpp:105
+
+// Test virtual method is diagnosted although not overridden in parent.
+class BI : public A {

aaron.ballman wrote:
> Typo: diagnosted -> diagnosed
oops, thanks.



Comment at: test/clang-tidy/bugprone-parent-virtual-call.cpp:113
+  int virt_1() override { return A::virt_1(); }
+  // CHECK-MESSAGES: :[[@LINE-1]]:34: warning: qualified function name 
A::virt_1 refers to a function not from a direct base class; did you mean 'BI'? 
[bugprone-parent-virtual-call]
+  // CHECK-FIXES:  int virt_1() override { return BI::virt_1(); }

aaron.ballman wrote:
> This seems like a false positive to me. Yes, the virtual function is 
> technically exposed in `BI`, but why is the programmer obligated to call that 
> one rather than the one from `A`, which is written in the source?
IMHO it's a matter of safety. Today virt_1() is not overridden in BI, but 
tomorrow someone will implement BI::virt_1() and it will silently lead to bugs 
or whatever.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44295



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44295: [clang-tidy] Detects and fixes calls to grand-...parent virtual methods instead of calls to parent's virtual methods

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: test/clang-tidy/bugprone-parent-virtual-call.cpp:105
+
+// Test virtual method is diagnosted although not overridden in parent.
+class BI : public A {

Typo: diagnosted -> diagnosed



Comment at: test/clang-tidy/bugprone-parent-virtual-call.cpp:113
+  int virt_1() override { return A::virt_1(); }
+  // CHECK-MESSAGES: :[[@LINE-1]]:34: warning: qualified function name 
A::virt_1 refers to a function not from a direct base class; did you mean 'BI'? 
[bugprone-parent-virtual-call]
+  // CHECK-FIXES:  int virt_1() override { return BI::virt_1(); }

This seems like a false positive to me. Yes, the virtual function is 
technically exposed in `BI`, but why is the programmer obligated to call that 
one rather than the one from `A`, which is written in the source?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44295



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44295: [clang-tidy] Detects and fixes calls to grand-...parent virtual methods instead of calls to parent's virtual methods

[Zinovy Nis via Phabricator via cfe-commits]

zinovy.nis updated this revision to Diff 138601.

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,139 @@
+// RUN: %check_clang_tidy %s bugprone-parent-virtual-call %t
+
+extern int foo();
+
+class A {
+public:
+  A() = default;
+  virtual ~A() = default;
+
+  virtual int virt_1() { return foo() + 1; }
+  virtual int virt_2() { return foo() + 2; }
+
+  int non_virt() { return foo() + 3; }
+  static int stat() { return foo() + 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: qualified function name A::virt_1 refers to a function not from a direct base class; did you mean 'B'? [bugprone-parent-virtual-call]
+  // 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: qualified function name A::virt_1 refers to a function not from a direct base class; did you mean 'B'? [bugprone-parent-virtual-call]
+  // 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: qualified function name A::virt_1 refers to a function not from a direct base class; did you mean 'C'? [bugprone-parent-virtual-call]
+  // CHECK-MESSAGES: :[[@LINE-2]]:48: warning: qualified function name B::virt_1 refers to a function not from a direct base class; did you mean 'C'? [bugprone-parent-virtual-call]
+  // 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: qualified function name A::virt_1 refers to a function not from a direct base class; did you mean 'C'? [bugprone-parent-virtual-call]
+  // CHECK-MESSAGES: :[[@LINE-2]]:48: warning: qualified function name B::virt_1 refers to a function not from a direct base class; did you mean 'C'? [bugprone-parent-virtual-call]
+  // 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: qualified function name A::virt_1 refers to a function not from a direct base class; did you mean 'BN'? [bugprone-parent-virtual-call]
+  // 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: qualified function name A::virt_1 refers to a function not from a direct base class; did you mean 'BN'? [bugprone-parent-virtual-call]
+  // 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 foo() + 1; }
+  virtual int virt_2() { return foo() + 2; }
+
+  int non_virt() { return foo() + 3; }
+  static int stat() { return foo() + 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: qualified function name AA::virt_1 refers to a function not from a direct base class; did you mean 'BB_1' or 'BB_2'? [bugprone-parent-virt

[PATCH] D44295: [clang-tidy] Detects and fixes calls to grand-...parent virtual methods instead of calls to parent's virtual methods

[Zinovy Nis via Phabricator via cfe-commits]

zinovy.nis added inline comments.



Comment at: test/clang-tidy/bugprone-parent-virtual-call.cpp:127
+// Just to instantiate DF.
+int bar() { return (new DF())->virt_1(); }

aaron.ballman wrote:
> What should happen in this case?
> ```
> struct Base {
>   virtual void f() {}
> };
> 
> struct Intermediary : Base {
> };
> 
> struct Derived : Intermediary {
>   void f() override {
> Base::f(); // I would expect this not to diagnose
>   }
> };
> ```
> This would be a reasonable test case to add.
Please see `// Test virtual method is diagnosted although not overridden in 
parent.` section in the test file.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44295



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44295: [clang-tidy] Detects and fixes calls to grand-...parent virtual methods instead of calls to parent's virtual methods

[Zinovy Nis via Phabricator via cfe-commits]

zinovy.nis updated this revision to Diff 138599.

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,139 @@
+// RUN: %check_clang_tidy %s bugprone-parent-virtual-call %t
+
+extern int foo();
+
+class A {
+public:
+  A() = default;
+  virtual ~A() = default;
+
+  virtual int virt_1() { return foo() + 1; }
+  virtual int virt_2() { return foo() + 2; }
+
+  int non_virt() { return foo() + 3; }
+  static int stat() { return foo() + 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: qualified function name A::virt_1 refers to a function not from a direct base class; did you mean 'B'? [bugprone-parent-virtual-call]
+  // 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: qualified function name A::virt_1 refers to a function not from a direct base class; did you mean 'B'? [bugprone-parent-virtual-call]
+  // 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: qualified function name A::virt_1 refers to a function not from a direct base class; did you mean 'C'? [bugprone-parent-virtual-call]
+  // CHECK-MESSAGES: :[[@LINE-2]]:48: warning: qualified function name B::virt_1 refers to a function not from a direct base class; did you mean 'C'? [bugprone-parent-virtual-call]
+  // 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: qualified function name A::virt_1 refers to a function not from a direct base class; did you mean 'C'? [bugprone-parent-virtual-call]
+  // CHECK-MESSAGES: :[[@LINE-2]]:48: warning: qualified function name B::virt_1 refers to a function not from a direct base class; did you mean 'C'? [bugprone-parent-virtual-call]
+  // 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: qualified function name A::virt_1 refers to a function not from a direct base class; did you mean 'BN'? [bugprone-parent-virtual-call]
+  // 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: qualified function name A::virt_1 refers to a function not from a direct base class; did you mean 'BN'? [bugprone-parent-virtual-call]
+  // 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 foo() + 1; }
+  virtual int virt_2() { return foo() + 2; }
+
+  int non_virt() { return foo() + 3; }
+  static int stat() { return foo() + 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: qualified function name AA::virt_1 refers to a function not from a direct base class; did you mean 'BB_1' or 'BB_2'? [bugprone-parent-virt

[PATCH] D44295: [clang-tidy] Detects and fixes calls to grand-...parent virtual methods instead of calls to parent's virtual methods

[Zinovy Nis via Phabricator via cfe-commits]

zinovy.nis updated this revision to Diff 138598.

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,139 @@
+// RUN: %check_clang_tidy %s bugprone-parent-virtual-call %t
+
+extern int foo();
+
+class A {
+public:
+  A() = default;
+  virtual ~A() = default;
+
+  virtual int virt_1() { return foo() + 1; }
+  virtual int virt_2() { return foo() + 2; }
+
+  int non_virt() { return foo() + 3; }
+  static int stat() { return foo() + 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: qualified function name A::virt_1 refers to a function not from a direct base class; did you mean 'B'? [bugprone-parent-virtual-call]
+  // 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: qualified function name A::virt_1 refers to a function not from a direct base class; did you mean 'B'? [bugprone-parent-virtual-call]
+  // 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: qualified function name A::virt_1 refers to a function not from a direct base class; did you mean 'C'? [bugprone-parent-virtual-call]
+  // CHECK-MESSAGES: :[[@LINE-2]]:48: warning: qualified function name B::virt_1 refers to a function not from a direct base class; did you mean 'C'? [bugprone-parent-virtual-call]
+  // 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: qualified function name A::virt_1 refers to a function not from a direct base class; did you mean 'C'? [bugprone-parent-virtual-call]
+  // CHECK-MESSAGES: :[[@LINE-2]]:48: warning: qualified function name B::virt_1 refers to a function not from a direct base class; did you mean 'C'? [bugprone-parent-virtual-call]
+  // 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: qualified function name A::virt_1 refers to a function not from a direct base class; did you mean 'BN'? [bugprone-parent-virtual-call]
+  // 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: qualified function name A::virt_1 refers to a function not from a direct base class; did you mean 'BN'? [bugprone-parent-virtual-call]
+  // 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 foo() + 1; }
+  virtual int virt_2() { return foo() + 2; }
+
+  int non_virt() { return foo() + 3; }
+  static int stat() { return foo() + 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: qualified function name AA::virt_1 refers to a function not from a direct base class; did you mean 'BB_1' or 'BB_2'? [bugprone-parent-virt

[PATCH] D44295: [clang-tidy] Detects and fixes calls to grand-...parent virtual methods instead of calls to parent's virtual methods

[Zinovy Nis via Phabricator via cfe-commits]

zinovy.nis added inline comments.



Comment at: clang-tidy/bugprone/ParentVirtualCallCheck.cpp:128
+  diag(Member->getQualifierLoc().getSourceRange().getBegin(),
+   "'%0' is a grand-parent's method, not parent's. Did you mean %1?")
+  << CalleeName << ParentsStr;

aaron.ballman wrote:
> The diagnostic should not contain punctuation aside from the question mark.
> 
> Also, the diagnostic uses some novel terminology in "grandparent's method". 
> How about: "qualified function name %0 refers to a function that is not 
> defined by a direct base class; did you mean %1?"
"is not defined by a direct base class" is not a correct proposal. Issues that 
this check finds are all about calling a grand-parent's method instead of 
parent's whether base class defines this method or not.

How about 

> Qualified function name %0 refers to a function not from a direct base class; 
> did you mean %1?

?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44295



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44295: [clang-tidy] Detects and fixes calls to grand-...parent virtual methods instead of calls to parent's virtual methods

[Zinovy Nis via Phabricator via cfe-commits]

zinovy.nis updated this revision to Diff 138597.

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,139 @@
+// RUN: %check_clang_tidy %s bugprone-parent-virtual-call %t
+
+extern int foo();
+
+class A {
+public:
+  A() = default;
+  virtual ~A() = default;
+
+  virtual int virt_1() { return foo() + 1; }
+  virtual int virt_2() { return foo() + 2; }
+
+  int non_virt() { return foo() + 3; }
+  static int stat() { return foo() + 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: Qualified function name A::virt_1 refers to a function not from a direct base class; did you mean 'B'? [bugprone-parent-virtual-call]
+  // 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: Qualified function name A::virt_1 refers to a function not from a direct base class; did you mean 'B'? [bugprone-parent-virtual-call]
+  // 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: Qualified function name A::virt_1 refers to a function not from a direct base class; did you mean 'C'? [bugprone-parent-virtual-call]
+  // CHECK-MESSAGES: :[[@LINE-2]]:48: warning: Qualified function name B::virt_1 refers to a function not from a direct base class; did you mean 'C'? [bugprone-parent-virtual-call]
+  // 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: Qualified function name A::virt_1 refers to a function not from a direct base class; did you mean 'C'? [bugprone-parent-virtual-call]
+  // CHECK-MESSAGES: :[[@LINE-2]]:48: warning: Qualified function name B::virt_1 refers to a function not from a direct base class; did you mean 'C'? [bugprone-parent-virtual-call]
+  // 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: Qualified function name A::virt_1 refers to a function not from a direct base class; did you mean 'BN'? [bugprone-parent-virtual-call]
+  // 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: Qualified function name A::virt_1 refers to a function not from a direct base class; did you mean 'BN'? [bugprone-parent-virtual-call]
+  // 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 foo() + 1; }
+  virtual int virt_2() { return foo() + 2; }
+
+  int non_virt() { return foo() + 3; }
+  static int stat() { return foo() + 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: Qualified function name AA::virt_1 refers to a function not from a direct base class; did you mean 'BB_1' or 'BB_2'? [bugprone-parent-virt

[PATCH] D44295: [clang-tidy] Detects and fixes calls to grand-...parent virtual methods instead of calls to parent's virtual methods

[Zinovy Nis via Phabricator via cfe-commits]

zinovy.nis updated this revision to Diff 138596.
zinovy.nis marked 10 inline comments as done.

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,127 @@
+// RUN: %check_clang_tidy %s bugprone-parent-virtual-call %t
+
+extern int foo();
+
+class A {
+public:
+  A() = default;
+  virtual ~A() = default;
+
+  virtual int virt_1() { return foo() + 1; }
+  virtual int virt_2() { return foo() + 2; }
+
+  int non_virt() { return foo() + 3; }
+  static int stat() { return foo() + 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 '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 '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 foo() + 1; }
+  virtual int virt_2() { return foo() + 2; }
+
+  int non_virt() { return foo() + 3; }
+  static int stat() { return foo() + 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 choice of parent class.
+};
+
+// Test templated classes.
+template  class BF : public A {
+public:
+  int virt_1() override { return A::virt_1() + 3; }
+};
+
+// Test templated parent class.
+class CF : public BF {
+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'?
+};
+
+// Test both templated class a

[PATCH] D44295: [clang-tidy] Detects and fixes calls to grand-...parent virtual methods instead of calls to parent's virtual methods

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh requested changes to this revision.
alexfh added inline comments.
This revision now requires changes to proceed.



Comment at: clang-tidy/bugprone/ParentVirtualCallCheck.cpp:24-25
+   const CXXRecordDecl *ThisClass) {
+  assert(Parent);
+  assert(ThisClass);
+  if (Parent->getCanonicalDecl() == ThisClass->getCanonicalDecl())

aaron.ballman wrote:
> You can drop these asserts.
... and make arguments const references ;)



Comment at: clang-tidy/bugprone/ParentVirtualCallCheck.cpp:44
+
+static std::list
+GetParentsByGrandParent(const CXXRecordDecl *GrandParent,

std::list is almost never a good choice due to large overhead, poor locality of 
the data, etc. Use std::vector instead.



Comment at: clang-tidy/bugprone/ParentVirtualCallCheck.cpp:53
+GrandParent,
+Base.getType()->getAsCXXRecordDecl()->getCanonicalDecl()))
+  result.push_back(

Pull this to a variable to avoid repetition.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44295



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44295: [clang-tidy] Detects and fixes calls to grand-...parent virtual methods instead of calls to parent's virtual methods

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang-tidy/bugprone/ParentVirtualCallCheck.cpp:24-25
+   const CXXRecordDecl *ThisClass) {
+  assert(Parent);
+  assert(ThisClass);
+  if (Parent->getCanonicalDecl() == ThisClass->getCanonicalDecl())

You can drop these asserts.



Comment at: clang-tidy/bugprone/ParentVirtualCallCheck.cpp:28-29
+return true;
+  const auto ClassIter = std::find_if(
+  ThisClass->bases_begin(), ThisClass->bases_end(), [=](auto &Base) {
+assert(Base.getType()->getAsCXXRecordDecl());

You can use `llvm::find_if()` for the range-based API instead.



Comment at: clang-tidy/bugprone/ParentVirtualCallCheck.cpp:30-32
+assert(Base.getType()->getAsCXXRecordDecl());
+return Parent->getCanonicalDecl() ==
+   Base.getType()->getAsCXXRecordDecl()->getCanonicalDecl();

Factor out `Base.getType()->getAs()` and use the pointer in both the assert and 
the comparison.



Comment at: clang-tidy/bugprone/ParentVirtualCallCheck.cpp:39-40
+const CXXRecordDecl *ThisClass) {
+  assert(Parent);
+  assert(ThisClass);
+  return ThisClass->isDerivedFrom(Parent);

And these asserts as well.

I don't think this function adds much value given that it's a one-liner; I'd 
hoist its functionality out to the caller.



Comment at: clang-tidy/bugprone/ParentVirtualCallCheck.cpp:56-60
+  }
+  return result;
+}
+
+static std::string GetNameAsString(const NamedDecl *Decl) {

This should be `getNameAsString()`



Comment at: clang-tidy/bugprone/ParentVirtualCallCheck.cpp:101-103
+  else {
+if (auto *TemplateSpecializationTypePtr =
+Result.Nodes.getNodeAs("castToType")) {

Convert this to an `else if` and elide the braces.



Comment at: clang-tidy/bugprone/ParentVirtualCallCheck.cpp:112
+
+  const auto Parents = GetParentsByGrandParent(CastToType, ThisType);
+  assert(!Parents.empty());

Do not use `auto` here as the type is not spelled out in the intialization.



Comment at: clang-tidy/bugprone/ParentVirtualCallCheck.cpp:128
+  diag(Member->getQualifierLoc().getSourceRange().getBegin(),
+   "'%0' is a grand-parent's method, not parent's. Did you mean %1?")
+  << CalleeName << ParentsStr;

The diagnostic should not contain punctuation aside from the question mark.

Also, the diagnostic uses some novel terminology in "grandparent's method". How 
about: "qualified function name %0 refers to a function that is not defined by 
a direct base class; did you mean %1?"



Comment at: test/clang-tidy/bugprone-parent-virtual-call.cpp:127
+// Just to instantiate DF.
+int bar() { return (new DF())->virt_1(); }

What should happen in this case?
```
struct Base {
  virtual void f() {}
};

struct Intermediary : Base {
};

struct Derived : Intermediary {
  void f() override {
Base::f(); // I would expect this not to diagnose
  }
};
```
This would be a reasonable test case to add.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44295



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44295: [clang-tidy] Detects and fixes calls to grand-...parent virtual methods instead of calls to parent's virtual methods

[Zinovy Nis via Phabricator via cfe-commits]

zinovy.nis marked an inline comment as done and an inline comment as not done.
zinovy.nis added inline comments.



Comment at: test/clang-tidy/bugprone-parent-virtual-call.cpp:115
+  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(); }

malcolm.parsons wrote:
> There isn't an F in scope.
> The base class isn't a dependent type here, so no template params are needed.
Finally I decided not to propose a fix for templated parent.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44295



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44295: [clang-tidy] Detects and fixes calls to grand-...parent virtual methods instead of calls to parent's virtual methods

[Zinovy Nis via Phabricator via cfe-commits]

zinovy.nis updated this revision to Diff 138254.

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,127 @@
+// RUN: %check_clang_tidy %s bugprone-parent-virtual-call %t
+
+extern int foo();
+
+class A {
+public:
+  A() = default;
+  virtual ~A() = default;
+
+  virtual int virt_1() { return foo() + 1; }
+  virtual int virt_2() { return foo() + 2; }
+
+  int non_virt() { return foo() + 3; }
+  static int stat() { return foo() + 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 '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 '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 foo() + 1; }
+  virtual int virt_2() { return foo() + 2; }
+
+  int non_virt() { return foo() + 3; }
+  static int stat() { return foo() + 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 choice of parent class.
+};
+
+// Test templated classes.
+template  class BF : public A {
+public:
+  int virt_1() override { return A::virt_1() + 3; }
+};
+
+// Test templated parent class.
+class CF : public BF {
+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'?
+};
+
+// Test both templated class and its parent class.
+template  class DF : pub

[PATCH] D44295: [clang-tidy] Detects and fixes calls to grand-...parent virtual methods instead of calls to parent's virtual methods

[Malcolm Parsons via Phabricator via cfe-commits]

malcolm.parsons added inline comments.



Comment at: test/clang-tidy/bugprone-parent-virtual-call.cpp:115
+  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(); }

There isn't an F in scope.
The base class isn't a dependent type here, so no template params are needed.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44295



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44295: [clang-tidy] Detects and fixes calls to grand-...parent virtual methods instead of calls to parent's virtual methods

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang-tidy/bugprone/ParentVirtualCallCheck.cpp:47
+const CXXRecordDecl *ThisClass) {
+
+  assert(GrandParent != nullptr);

Please remove empty line.



Comment at: clang-tidy/bugprone/ParentVirtualCallCheck.cpp:116
+
+  if (IsParentOf(CastToType, ThisType)) {
+return;

No need for brackets.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44295



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44295: [clang-tidy] Detects and fixes calls to grand-...parent virtual methods instead of calls to parent's virtual methods

[Zinovy Nis via Phabricator via cfe-commits]

zinovy.nis updated this revision to Diff 137953.

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,129 @@
+// RUN: %check_clang_tidy %s bugprone-parent-virtual-call %t
+
+extern int foo();
+
+class A {
+public:
+  A() = default;
+  virtual ~A() = default;
+
+  virtual int virt_1() { return foo() + 1; }
+  virtual int virt_2() { return foo() + 2; }
+
+  int non_virt() { return foo() + 3; }
+  static int stat() { return foo() + 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 '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 '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 foo() + 1; }
+  virtual int virt_2() { return foo() + 2; }
+
+  int non_virt() { return foo() + 3; }
+  static int stat() { return foo() + 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 choice of parent class.
+};
+
+// Test templated classes.
+template  class BF : public A {
+public:
+  int virt_1() override { return A::virt_1() + 3; }
+};
+
+// Test templated parent class.
+class CF : public BF {
+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(); }
+};
+
+// Test b

[PATCH] D44295: [clang-tidy] Detects and fixes calls to grand-...parent virtual methods instead of calls to parent's virtual methods

[Zinovy Nis via Phabricator via cfe-commits]

zinovy.nis updated this revision to Diff 137952.

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,129 @@
+// RUN: %check_clang_tidy %s bugprone-parent-virtual-call %t
+
+extern int foo();
+
+class A {
+public:
+  A() = default;
+  virtual ~A() = default;
+
+  virtual int virt_1() { return foo() + 1; }
+  virtual int virt_2() { return foo() + 2; }
+
+  int non_virt() { return foo() + 3; }
+  static int stat() { return foo() + 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 '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 '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 foo() + 1; }
+  virtual int virt_2() { return foo() + 2; }
+
+  int non_virt() { return foo() + 3; }
+  static int stat() { return foo() + 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 choice of parent class.
+};
+
+// Test templated classes.
+template  class BF : public A {
+public:
+  int virt_1() override { return A::virt_1() + 3; }
+};
+
+// Test templated parent class.
+class CF : public BF {
+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(); }
+};
+
+// Test b

[PATCH] D44295: [clang-tidy] Detects and fixes calls to grand-...parent virtual methods instead of calls to parent's virtual methods

[Zinovy Nis via Phabricator via cfe-commits]

zinovy.nis updated this revision to Diff 137951.

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,129 @@
+// RUN: %check_clang_tidy %s bugprone-parent-virtual-call %t
+
+extern int foo();
+
+class A {
+public:
+  A() = default;
+  virtual ~A() = default;
+
+  virtual int virt_1() { return foo() + 1; }
+  virtual int virt_2() { return foo() + 2; }
+
+  int non_virt() { return foo() + 3; }
+  static int stat() { return foo() + 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 '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 '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 foo() + 1; }
+  virtual int virt_2() { return foo() + 2; }
+
+  int non_virt() { return foo() + 3; }
+  static int stat() { return foo() + 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 choice of parent class.
+};
+
+// Test templated classes.
+template  class BF : public A {
+public:
+  int virt_1() override { return A::virt_1() + 3; }
+};
+
+// Test templated parent class.
+class CF : public BF {
+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(); }
+};
+
+// Test b

[PATCH] D44295: [clang-tidy] Detects and fixes calls to grand-...parent virtual methods instead of calls to parent's virtual methods

[Zinovy Nis via Phabricator via cfe-commits]

zinovy.nis marked an inline comment as done.
zinovy.nis added inline comments.



Comment at: test/clang-tidy/bugprone-parent-virtual-call.cpp:125
+  // 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(); }
+};

malcolm.parsons wrote:
> Does this fixit compile?
Thanks for pointing. Hope I fixed this case in the latest patch. Please have a 
look at it.



Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44295



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44295: [clang-tidy] Detects and fixes calls to grand-...parent virtual methods instead of calls to parent's virtual methods

[Malcolm Parsons via Phabricator via cfe-commits]

malcolm.parsons added inline comments.



Comment at: test/clang-tidy/bugprone-parent-virtual-call.cpp:125
+  // 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(); }
+};

Does this fixit compile?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44295



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44295: [clang-tidy] Detects and fixes calls to grand-...parent virtual methods instead of calls to parent's virtual methods

[Zinovy Nis via Phabricator via cfe-commits]

zinovy.nis updated this revision to Diff 137911.

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,129 @@
+// RUN: %check_clang_tidy %s bugprone-parent-virtual-call %t
+
+extern int foo();
+
+class A {
+public:
+  A() = default;
+  virtual ~A() = default;
+
+  virtual int virt_1() { return foo() + 1; }
+  virtual int virt_2() { return foo() + 2; }
+
+  int non_virt() { return foo() + 3; }
+  static int stat() { return foo() + 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 foo() + 1; }
+  virtual int virt_2() { return foo() + 2; }
+
+  int non_virt() { return foo() + 3; }
+  static int stat() { return foo() + 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 choice of parent class.
+};
+
+// Test templated classes.
+template  class BF : public A {
+public:
+  int virt_1() override { return A::virt_1() + 3; }
+};
+
+// Test templated parent class.
+class CF : public BF {
+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() over

[PATCH] D44295: [clang-tidy] Detects and fixes calls to grand-...parent virtual methods instead of calls to parent's virtual methods

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang-tidy/bugprone/ParentVirtualCallCheck.cpp:12
+
+#include 
+#include 

Please run Clang-format and remove empty line between headers.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44295



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44295: [clang-tidy] Detects and fixes calls to grand-...parent virtual methods instead of calls to parent's virtual methods

[Malcolm Parsons via Phabricator via cfe-commits]

malcolm.parsons added inline comments.



Comment at: clang-tidy/bugprone/ParentVirtualCallCheck.cpp:152
+Member->getQualifierLoc().getSourceRange(),
+GetNameAsString(*(Parents.front())) + "::");
+  }

zinovy.nis wrote:
> malcolm.parsons wrote:
> > What does this do for templated parent classes?
> Please see my last test case in updated revision.
Also, a templated parent class of a templated class.
```
template 
class DF : public BF {
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(); }
};
```


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44295



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44295: [clang-tidy] Detects and fixes calls to grand-...parent virtual methods instead of calls to parent's virtual methods

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang-tidy/bugprone/ParentVirtualCallCheck.cpp:26
+
+bool IsParentOf(const CXXRecordDecl *Parent, const CXXRecordDecl *ThisClass) {
+  assert(Parent);

Please make this function static and remove anonymous namespace.



Comment at: clang-tidy/bugprone/ParentVirtualCallCheck.cpp:40
+
+bool IsRecursiveParentOf(const CXXRecordDecl *Parent,
+ const CXXRecordDecl *ThisClass) {

Please make this function static and remove anonymous namespace.



Comment at: clang-tidy/bugprone/ParentVirtualCallCheck.cpp:48
+std::list
+GetParentsByGrandParent(const CXXRecordDecl *GrandParent,
+const CXXRecordDecl *ThisClass) {

Please make this function static and remove anonymous namespace.



Comment at: clang-tidy/bugprone/ParentVirtualCallCheck.cpp:66
+// PrintingPolicy for anonymous namespaces.
+std::string GetNameAsString(const NamedDecl &Decl) {
+  PrintingPolicy PP(Decl.getASTContext().getPrintingPolicy());

Please make this function static and remove anonymous namespace.



Comment at: clang-tidy/bugprone/ParentVirtualCallCheck.cpp:91
+void ParentVirtualCallCheck::check(const MatchFinder::MatchResult &Result) {
+
+  const auto *MatchedDecl = Result.Nodes.getNodeAs("call");

Please remove this line.



Comment at: docs/ReleaseNotes.rst:76
+
+  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

Please make description here and first statement in documentation same.



Comment at: docs/clang-tidy/checks/bugprone-parent-virtual-call.rst:9
+
+class A {
+...

Please add .. code-block:: c++


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44295



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44295: [clang-tidy] Detects and fixes calls to grand-...parent virtual methods instead of calls to parent's virtual methods

[Zinovy Nis via Phabricator via cfe-commits]

zinovy.nis updated this revision to Diff 137787.

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 choice of parent class.
+};
+
+// Test templated classes.
+template  class BF : public A {
+public:
+  int virt_1() override { return A::virt_1() + 3; }
+};
+
+class CF : public BF {
+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

[PATCH] D44295: [clang-tidy] Detects and fixes calls to grand-...parent virtual methods instead of calls to parent's virtual methods

[Zinovy Nis via Phabricator via cfe-commits]

zinovy.nis added inline comments.



Comment at: clang-tidy/bugprone/ParentVirtualCallCheck.cpp:152
+Member->getQualifierLoc().getSourceRange(),
+GetNameAsString(*(Parents.front())) + "::");
+  }

malcolm.parsons wrote:
> What does this do for templated parent classes?
Please see my last test case in updated revision.



Comment at: docs/clang-tidy/checks/bugprone-parent-virtual-call.rst:6
+
+FIXME: Describe what patterns does the check detect and why. Give examples.

malcolm.parsons wrote:
> FIXME.
Oops. Thanks!



Comment at: test/clang-tidy/bugprone-parent-virtual-call.cpp:102
+  // 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.
+};

malcolm.parsons wrote:
> typo: choice
Thanks!


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44295



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44295: [clang-tidy] Detects and fixes calls to grand-...parent virtual methods instead of calls to parent's virtual methods

[Zinovy Nis via Phabricator via cfe-commits]

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 BF : public A {
+public:
+  int virt_1() override { return A::virt_1() + 3; }
+};
+
+class CF : public BF {
+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

[PATCH] D44295: [clang-tidy] Detects and fixes calls to grand-...parent virtual methods instead of calls to parent's virtual methods

[Zinovy Nis via Phabricator via cfe-commits]

zinovy.nis updated this revision to Diff 137783.

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 choice of parent class.
+};
+
+// Test templated classes.
+template  class BF : public A {
+public:
+  int virt_1() override { return A::virt_1() + 3; }
+};
+
+class CF : public BF {
+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