[ASTMatchers] new forEachOverriden matcher

[courbet]

Summary

Matches methods overridden by the given method.

[mgrang]

include/clang/ASTMatchers/ASTMatchers.h

3775

Extraneous ", and" at the end of comment.

[courbet]

Fix typo in doc.

[aaron.ballman]

Please run clang\docs\tools\dump_ast_matchers.py to regenerate the documentation as well.

include/clang/ASTMatchers/ASTMatchers.h

3780

Can you range-ify this for loop?

unittests/ASTMatchers/ASTMatchersTest.cpp

2084 ↗

(On Diff #54462)

Can you write the tests such that they don't leak?

[courbet]

unittests/ASTMatchers/ASTMatchersTest.cpp

2084 ↗

(On Diff #54462)

matchAndVerifyResultConditionally deletes them:

template <typename T>
testing::AssertionResult
matchAndVerifyResultConditionally(const std::string &Code, const T &AMatcher,
                                  BoundNodesCallback *FindResultVerifier,
                                  bool ExpectResult) {
  std::unique_ptr<BoundNodesCallback> ScopedVerifier(FindResultVerifier);

I'll send you a (separate) patch for taking a unique_ptr in.

[courbet]

include/clang/ASTMatchers/ASTMatchers.h

3780

I could do that , but it would require adding:

const CXXMethodVector* ASTContext::overridden_methods() const;

And plumbing it through. (or did I miss something ?)

Let me know what you think.

[aaron.ballman]

include/clang/ASTMatchers/ASTMatchers.h

3780

I think it's an oversight that we don't have the range form of that function, and this seems like a reasonable use case to add it.

[courbet]

• add overridden_methods() to CXXMethodDecl and plumbing.
• Use range-based for loop for iterating over overridden methods.

[courbet]

Regenerate doc.

[sbenza]

lib/AST/ASTContext.cpp

1280

It would be simpler to return ArrayRef<const CXXMethodDecl*>.
That way the caller doesn't need to deal with a potential null pointer or with the custom vector type.

[courbet]

overridden_methods return ArrayRef instead of TinyPtrVector*

[sbenza]

include/clang/AST/DeclCXX.h

1832

Return type should have have toplevel const.

lib/AST/ASTContext.cpp

1256–1257

I would invert the calls here.
That is, make overridden_methods_begin/_end call overridden_methods() instead, and have overridden_methods() be the one that does the lookup.
This way we have a single place where the lookup happens. It would also make overridden_methods() faster.

[courbet]

implement overridden_methods_begin()/end() in terms of overridden_methods().

[aaron.ballman]

include/clang/AST/ASTContext.h

824

This is too tight of a coupling to the underlying datatype. It should return iterator_range<overridden_cxx_method_iterator>

include/clang/AST/DeclCXX.h

1832

This should return iterator_range<method_iterator>

[courbet]

include/clang/AST/ASTContext.h

824

This is the exact opposite of the change that Samuel just requested (implementing the iterators in term of the ArrayRef getter). I don't have a strong opinion on this, but could you two agree on the desired API ?

[aaron.ballman]

include/clang/AST/ASTContext.h

824

I hadn't noticed that we disagreed, sorry for the conflicting advice. However, I strongly think that we should use an abstraction, and that ArrayRef is too concrete. FWIW, we use iterator_range in almost every other case in the code base when we refactored to rangify code.

[courbet]

include/clang/AST/ASTContext.h

824

Samuel, are you OK with this (reverting the last change and using iterator_range instead of ArrayRef) ?

[courbet]

Use iterator_range instead of ArrayRef for overridden_methods

[aaron.ballman]

LGTM, with one minor nit. Thank you for working on this!

include/clang/ASTMatchers/ASTMatchers.h

3780

Nit: the * should bind to Overridden instead of auto. (May just want to clang-format the patch.)

[courbet]

cometics

[courbet]

Thanks. Could please you submit this for me ?

include/clang/ASTMatchers/ASTMatchers.h

3780

Thanks for the catch. Unfortunately there are a lot of errors in the file that prevent me to run clang-format. Any hint to handle that efficiently ?

[aaron.ballman]

include/clang/ASTMatchers/ASTMatchers.h

3780

There's a script that you can use to reformat just the contents of a diff: http://clang.llvm.org/docs/ClangFormat.html#script-for-patch-reformatting

[courbet]

Rebase on origin/master.

[courbet]

clang-format diff

include/clang/ASTMatchers/ASTMatchers.h

3780

Thanks for the pointer !

[sbenza]

include/clang/AST/ASTContext.h

824

Sure. Sorry about that.
The main idea was to use a range-like API instead of returning a nullable pointer to a range.

[courbet]

Rebase on origin/master

[courbet]

@sbenza @aaron.ballman I don't have write access. Can one of you submit this on my behalf ? Thanks !

[courbet]

This comment was removed by courbet.

[courbet]

This was submitted as r272386.

docs/LibASTMatchersReference.html

 
 
 <tr><td>Matcher&lt;<a href="http://clang.llvm.org/doxygen/classclang_1_1CXXMemberCallExpr.html">CXXMemberCallExpr</a>&gt;</td><td class="name" onclick="toggle('thisPointerType0')"><a name="thisPointerType0Anchor">thisPointerType</a></td><td>Matcher&lt;<a href="http://clang.llvm.org/doxygen/classclang_1_1QualType.html">QualType</a>&gt; InnerMatcher</td></tr>
 <tr><td colspan="4" class="doc" id="thisPointerType0"><pre>Matches if the expression's type either matches the specified
 matcher, or is a pointer to a type that matches the InnerMatcher.
 </pre></td></tr>
 
 
+<tr><td>Matcher&lt;<a href="http://clang.llvm.org/doxygen/classclang_1_1CXXMethodDecl.html">CXXMethodDecl</a>&gt;</td><td class="name" onclick="toggle('forEachOverridden0')"><a name="forEachOverridden0Anchor">forEachOverridden</a></td><td>Matcher&lt;<a href="http://clang.llvm.org/doxygen/classclang_1_1CXXMethodDecl.html">CXXMethodDecl</a>&gt; InnerMatcher</td></tr>
+<tr><td colspan="4" class="doc" id="forEachOverridden0"><pre>Matches each method overriden by the given method. This matcher may
+produce multiple matches.
+
+Given
+  class A { virtual void f(); };
+  class B : public A { void f(); };
+  class C : public B { void f(); };
+cxxMethodDecl(ofClass(hasName("C")),
+              forEachOverridden(cxxMethodDecl().bind("b"))).bind("d")
+  matches once, with "b" binding "A::f" and "d" binding "C::f" (Note
+  that B::f is not overridden by C::f).
+
+The check can produce multiple matches in case of multiple inheritance, e.g.
+  class A1 { virtual void f(); };
+  class A2 { virtual void f(); };
+  class C : public A1, public A2 { void f(); };
+cxxMethodDecl(ofClass(hasName("C")),
+              forEachOverridden(cxxMethodDecl().bind("b"))).bind("d")
+  matches twice, once with "b" binding "A1::f" and "d" binding "C::f", and
+  once with "b" binding "A2::f" and "d" binding "C::f".
+</pre></td></tr>
+
+
 <tr><td>Matcher&lt;<a href="http://clang.llvm.org/doxygen/classclang_1_1CXXMethodDecl.html">CXXMethodDecl</a>&gt;</td><td class="name" onclick="toggle('ofClass0')"><a name="ofClass0Anchor">ofClass</a></td><td>Matcher&lt;<a href="http://clang.llvm.org/doxygen/classclang_1_1CXXRecordDecl.html">CXXRecordDecl</a>&gt; InnerMatcher</td></tr>
 <tr><td colspan="4" class="doc" id="ofClass0"><pre>Matches the class declaration that the given method declaration
 belongs to.
 
 FIXME: Generalize this for other kinds of declarations.
 FIXME: What other kind of declarations would we need to generalize
 this to?
 

include/clang/AST/ASTContext.h

public:
   typedef CXXMethodVector::const_iterator overridden_cxx_method_iterator;
   overridden_cxx_method_iterator
   overridden_methods_begin(const CXXMethodDecl *Method) const;
 
   overridden_cxx_method_iterator
   overridden_methods_end(const CXXMethodDecl *Method) const;
 
   unsigned overridden_methods_size(const CXXMethodDecl *Method) const;
+  typedef llvm::iterator_range<overridden_cxx_method_iterator>
+      overridden_method_range;
+  overridden_method_range overridden_methods(const CXXMethodDecl *Method) const;
 
   /// \brief Note that the given C++ \p Method overrides the given \p
   /// Overridden method.
   void addOverriddenMethod(const CXXMethodDecl *Method, 
                            const CXXMethodDecl *Overridden);
 
   /// \brief Return C++ or ObjC overridden methods for the given \p Method.
   ///

include/clang/AST/DeclCXX.h

 /// \brief Defines the C++ Decl subclasses, other than those for templates
 /// (found in DeclTemplate.h) and friends (in DeclFriend.h).
 ///
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CLANG_AST_DECLCXX_H
 #define LLVM_CLANG_AST_DECLCXX_H
 
+#include "clang/AST/ASTContext.h"
 #include "clang/AST/ASTUnresolvedSet.h"
 #include "clang/AST/Attr.h"
 #include "clang/AST/Decl.h"
 #include "clang/AST/Expr.h"
 #include "clang/AST/LambdaCapture.h"
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/PointerIntPair.h"
 #include "llvm/Support/Compiler.h"
public:
   ///
   void addOverriddenMethod(const CXXMethodDecl *MD);
 
   typedef const CXXMethodDecl *const* method_iterator;
 
   method_iterator begin_overridden_methods() const;
   method_iterator end_overridden_methods() const;
   unsigned size_overridden_methods() const;
+  typedef ASTContext::overridden_method_range overridden_method_range;
+  overridden_method_range overridden_methods() const;
 
   /// Returns the parent of this method declaration, which
   /// is the class in which this method is defined.
   const CXXRecordDecl *getParent() const {
     return cast<CXXRecordDecl>(FunctionDecl::getParent());
   }
 
   /// Returns the parent of this method declaration, which

include/clang/ASTMatchers/ASTMatchers.h

 /// \endcode
 AST_MATCHER_P(CXXMethodDecl, ofClass,
               internal::Matcher<CXXRecordDecl>, InnerMatcher) {
   const CXXRecordDecl *Parent = Node.getParent();
   return (Parent != nullptr &&
           InnerMatcher.matches(*Parent, Finder, Builder));
 }
 
+/// \brief Matches each method overriden by the given method. This matcher may
+/// produce multiple matches.
+///
+/// Given
+/// \code
+///   class A { virtual void f(); };
+///   class B : public A { void f(); };
+///   class C : public B { void f(); };
+/// \endcode
+/// cxxMethodDecl(ofClass(hasName("C")),
+///               forEachOverridden(cxxMethodDecl().bind("b"))).bind("d")
+///   matches once, with "b" binding "A::f" and "d" binding "C::f" (Note
+///   that B::f is not overridden by C::f).
+///
+/// The check can produce multiple matches in case of multiple inheritance, e.g.
+/// \code
+///   class A1 { virtual void f(); };
+///   class A2 { virtual void f(); };
+///   class C : public A1, public A2 { void f(); };
+/// \endcode
+/// cxxMethodDecl(ofClass(hasName("C")),
+///               forEachOverridden(cxxMethodDecl().bind("b"))).bind("d")
+///   matches twice, once with "b" binding "A1::f" and "d" binding "C::f", and
+///   once with "b" binding "A2::f" and "d" binding "C::f".
+AST_MATCHER_P(CXXMethodDecl, forEachOverridden,
+              internal::Matcher<CXXMethodDecl>, InnerMatcher) {
+  BoundNodesTreeBuilder Result;
+  bool Matched = false;
+  for (const auto *Overridden : Node.overridden_methods()) {
+    BoundNodesTreeBuilder OverriddenBuilder(*Builder);
+    const bool OverriddenMatched =
+        InnerMatcher.matches(*Overridden, Finder, &OverriddenBuilder);
+    if (OverriddenMatched) {
+      Matched = true;
+      Result.addMatch(OverriddenBuilder);
+    }
+  }
+  *Builder = std::move(Result);
+  return Matched;
+}
+
 /// \brief Matches if the given method declaration is virtual.
 ///
 /// Given
 /// \code
 ///   class A {
 ///    public:
 ///     virtual void x();
 ///   };

lib/AST/ASTContext.cpp

void ASTContext::setInstantiatedFromUnnamedFieldDecl(FieldDecl *Inst,
   assert(!Tmpl->getDeclName() && "Template field decl is not unnamed");
   assert(!InstantiatedFromUnnamedFieldDecl[Inst] &&
          "Already noted what unnamed field was instantiated from");
 
   InstantiatedFromUnnamedFieldDecl[Inst] = Tmpl;
 }
 
 ASTContext::overridden_cxx_method_iterator
 ASTContext::overridden_methods_begin(const CXXMethodDecl *Method) const {
-  llvm::DenseMap<const CXXMethodDecl *, CXXMethodVector>::const_iterator Pos
-    = OverriddenMethods.find(Method->getCanonicalDecl());
+  llvm::DenseMap<const CXXMethodDecl *, CXXMethodVector>::const_iterator Pos =
+      OverriddenMethods.find(Method->getCanonicalDecl());
   if (Pos == OverriddenMethods.end())
     return nullptr;
-
   return Pos->second.begin();
 }
 
 ASTContext::overridden_cxx_method_iterator
 ASTContext::overridden_methods_end(const CXXMethodDecl *Method) const {
-  llvm::DenseMap<const CXXMethodDecl *, CXXMethodVector>::const_iterator Pos
-    = OverriddenMethods.find(Method->getCanonicalDecl());
+  llvm::DenseMap<const CXXMethodDecl *, CXXMethodVector>::const_iterator Pos =
+      OverriddenMethods.find(Method->getCanonicalDecl());
   if (Pos == OverriddenMethods.end())
     return nullptr;
-
   return Pos->second.end();
 }
 
 unsigned
 ASTContext::overridden_methods_size(const CXXMethodDecl *Method) const {
-  llvm::DenseMap<const CXXMethodDecl *, CXXMethodVector>::const_iterator Pos
-    = OverriddenMethods.find(Method->getCanonicalDecl());
+  llvm::DenseMap<const CXXMethodDecl *, CXXMethodVector>::const_iterator Pos =
+      OverriddenMethods.find(Method->getCanonicalDecl());
   if (Pos == OverriddenMethods.end())
     return 0;
-
   return Pos->second.size();
 }
 
+ASTContext::overridden_method_range
+ASTContext::overridden_methods(const CXXMethodDecl *Method) const {
+  return overridden_method_range(overridden_methods_begin(Method),
+                                 overridden_methods_end(Method));
+}
+
 void ASTContext::addOverriddenMethod(const CXXMethodDecl *Method, 
                                      const CXXMethodDecl *Overridden) {
   assert(Method->isCanonicalDecl() && Overridden->isCanonicalDecl());
   OverriddenMethods[Method].push_back(Overridden);
 }
 
 void ASTContext::getOverriddenMethods(
                       const NamedDecl *D,

lib/AST/DeclCXX.cpp

CXXMethodDecl::method_iterator CXXMethodDecl::end_overridden_methods() const {
   return getASTContext().overridden_methods_end(this);
 }
 
 unsigned CXXMethodDecl::size_overridden_methods() const {
   if (isa<CXXConstructorDecl>(this)) return 0;
   return getASTContext().overridden_methods_size(this);
 }
 
+CXXMethodDecl::overridden_method_range
+CXXMethodDecl::overridden_methods() const {
+  if (isa<CXXConstructorDecl>(this))
+    return overridden_method_range(nullptr, nullptr);
+  return getASTContext().overridden_methods(this);
+}
+
 QualType CXXMethodDecl::getThisType(ASTContext &C) const {
   // C++ 9.3.2p1: The type of this in a member function of a class X is X*.
   // If the member function is declared const, the type of this is const X*,
   // if the member function is declared volatile, the type of this is
   // volatile X*, and if the member function is declared const volatile,
   // the type of this is const volatile X*.
 
   assert(isInstance() && "No 'this' for static methods!");

unittests/ASTMatchers/ASTMatchersTraversalTest.cpp

EXPECT_TRUE(
     matches(
       CppString1,
       returnStmt(forFunction(hasName("operator()")),
                  has(integerLiteral()))));
   EXPECT_TRUE(matches(CppString2, returnStmt(forFunction(hasName("F2")))));
   EXPECT_TRUE(notMatches(CppString2, returnStmt(forFunction(hasName("F")))));
 }
 
+TEST(Matcher, ForEachOverriden) {
+  const auto ForEachOverriddenInClass = [](const char *ClassName) {
+    return cxxMethodDecl(ofClass(hasName(ClassName)), isVirtual(),
+                         forEachOverridden(cxxMethodDecl().bind("overridden")))
+        .bind("override");
+  };
+  constexpr const char Code1[] = "class A { virtual void f(); };"
+                                 "class B : public A { void f(); };"
+                                 "class C : public B { void f(); };";
+  // C::f overrides A::f.
+  EXPECT_TRUE(matchAndVerifyResultTrue(
+      Code1, ForEachOverriddenInClass("C"),
+      llvm::make_unique<VerifyIdIsBoundTo<CXXMethodDecl>>("override", "f", 1)));
+  EXPECT_TRUE(matchAndVerifyResultTrue(
+      Code1, ForEachOverriddenInClass("C"),
+      llvm::make_unique<VerifyIdIsBoundTo<CXXMethodDecl>>("overridden", "f",
+                                                          1)));
+  // B::f overrides A::f.
+  EXPECT_TRUE(matchAndVerifyResultTrue(
+      Code1, ForEachOverriddenInClass("B"),
+      llvm::make_unique<VerifyIdIsBoundTo<CXXMethodDecl>>("override", "f", 1)));
+  EXPECT_TRUE(matchAndVerifyResultTrue(
+      Code1, ForEachOverriddenInClass("B"),
+      llvm::make_unique<VerifyIdIsBoundTo<CXXMethodDecl>>("overridden", "f",
+                                                          1)));
+  // A::f overrides nothing.
+  EXPECT_TRUE(notMatches(Code1, ForEachOverriddenInClass("A")));
+
+  constexpr const char Code2[] =
+      "class A1 { virtual void f(); };"
+      "class A2 { virtual void f(); };"
+      "class B : public A1, public A2 { void f(); };";
+  // B::f overrides A1::f and A2::f. This produces two matches.
+  EXPECT_TRUE(matchAndVerifyResultTrue(
+      Code2, ForEachOverriddenInClass("B"),
+      llvm::make_unique<VerifyIdIsBoundTo<CXXMethodDecl>>("override", "f", 2)));
+  EXPECT_TRUE(matchAndVerifyResultTrue(
+      Code2, ForEachOverriddenInClass("B"),
+      llvm::make_unique<VerifyIdIsBoundTo<CXXMethodDecl>>("overridden", "f",
+                                                          2)));
+  // A1::f overrides nothing.
+  EXPECT_TRUE(notMatches(Code2, ForEachOverriddenInClass("A1")));
+}
+
 } // namespace ast_matchers
 } // namespace clang