[PATCH] D54408: [ASTMatchers] Add matchers available through casting to derived

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

In D54408#2056251 , @steveire wrote:

> @aaron.ballman  I think we agreed in Belfast in November (after the most 
> recent comment) to get this in as it is and not be as draconian about `auto`. 
> Is anything blocking this?


Nothing is blocking this, but I think we may have slightly different 
understandings of what we agreed to. Reviewers asking for code to follow the 
coding guidelines is not draconian; it's expected for code to follow the coding 
guidelines and for patch authors to field requests for changes like these. I 
re-reviewed the things I was asking for changes on and have added a comment 
where I definitely insist on changes. LGTM with that nit fixed though.




Comment at: lib/ASTMatchers/Dynamic/Registry.cpp:705
+getDerivedResults(ast_type_traits::ASTNodeKind StaticType, StringRef Name) {
+  auto NestedResult = getMatchingMatchersImpl(StaticType, ExactOnly);
+

aaron.ballman wrote:
> Don't use `auto` here please.
Please change this use of `auto` -- I have no idea what the type is for 
`NestedResult` without hunting for it and so I have no idea what the type is 
for `Item` and whether using a const reference is reasonable or not (after 
doing the hunting, it looks fine though).


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D54408/new/

https://reviews.llvm.org/D54408



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

[PATCH] D54408: [ASTMatchers] Add matchers available through casting to derived

[Stephen Kelly via Phabricator via cfe-commits]

steveire added a comment.

@aaron.ballman  I think we agreed in Belfast in November (after the most recent 
comment) to get this in as it is and not be as draconian about `auto`. Is 
anything blocking this?


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D54408/new/

https://reviews.llvm.org/D54408



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

[PATCH] D54408: [ASTMatchers] Add matchers available through casting to derived

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D54408#1659301 , @Abpostelnicu 
wrote:

> @aaron.ballman 
>  I think the auto usage improves and simplifies the code, however, as I would 
> really like to see this code landed, I would be ok to help Stephen to finish 
> this work and replace auto by the "old" way. Do you think we can have this 
> approved if we make the changes mentioned earlier?


Nothing jumps out at me as blocking this, aside from completing the usual 
review activities. Have at it!


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D54408/new/

https://reviews.llvm.org/D54408



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

[PATCH] D54408: [ASTMatchers] Add matchers available through casting to derived

[Andi via Phabricator via cfe-commits]

Abpostelnicu added a comment.
Herald added a project: clang.

@aaron.ballman 
I think the auto usage improves and simplifies the code, however, as I would 
really like to see this code landed, I would be ok to help Steven to finish 
this work and replace auto by the "old" way. Do you think we can have this 
approved if we make the changes mentioned earlier?


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D54408/new/

https://reviews.llvm.org/D54408



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

[PATCH] D54408: [ASTMatchers] Add matchers available through casting to derived

[Stephen Kelly via Phabricator via cfe-commits]

steveire updated this revision to Diff 179953.
steveire added a comment.

Case


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D54408/new/

https://reviews.llvm.org/D54408

Files:
  lib/ASTMatchers/Dynamic/Registry.cpp
  unittests/ASTMatchers/Dynamic/RegistryTest.cpp

Index: unittests/ASTMatchers/Dynamic/RegistryTest.cpp
===
--- unittests/ASTMatchers/Dynamic/RegistryTest.cpp
+++ unittests/ASTMatchers/Dynamic/RegistryTest.cpp
@@ -461,6 +461,16 @@
   "Matcher "
   "hasDescendant(Matcher)"));
 
+  CompVector FuncDeclComps = getCompletions("functionDecl", 0);
+
+  EXPECT_TRUE(!FuncDeclComps.empty());
+
+  // Exclude bases
+  EXPECT_TRUE(!hasCompletion(FuncDeclComps, "namedDecl("));
+  EXPECT_TRUE(!hasCompletion(FuncDeclComps, "valueDecl("));
+  EXPECT_TRUE(!hasCompletion(FuncDeclComps, "declaratorDecl("));
+  EXPECT_TRUE(!hasCompletion(FuncDeclComps, "functionDecl("));
+
   CompVector WhileComps = getCompletions("whileStmt", 0);
 
   EXPECT_TRUE(hasCompletion(WhileComps, "hasBody(",
@@ -569,13 +579,13 @@
   EXPECT_TRUE(Contains(Matchers, "isPublic()"));
   EXPECT_TRUE(Contains(Matchers, "hasName()"));
   EXPECT_TRUE(Contains(Matchers, "parameterCountIs()"));
+  EXPECT_TRUE(Contains(Matchers, "cxxMethodDecl(isOverride())"));
 
   EXPECT_FALSE(Contains(Matchers, "namedDecl()"));
   EXPECT_FALSE(Contains(Matchers, "valueDecl()"));
   EXPECT_FALSE(Contains(Matchers, "declaratorDecl()"));
   EXPECT_FALSE(Contains(Matchers, "functionDecl()"));
-
-  EXPECT_TRUE(Contains(Matchers, "cxxMethodDecl()"));
+  EXPECT_FALSE(Contains(Matchers, "cxxMethodDecl()"));
 }
 
 } // end anonymous namespace
Index: lib/ASTMatchers/Dynamic/Registry.cpp
===
--- lib/ASTMatchers/Dynamic/Registry.cpp
+++ lib/ASTMatchers/Dynamic/Registry.cpp
@@ -647,17 +647,77 @@
   }
 }
 
+llvm::Optional>
+getNodeConstructorType(MatcherCtor TargetCtor) {
+  const auto  = RegistryData->nodeConstructors();
+  auto It = llvm::find_if(
+  Ctors, [TargetCtor](const NodeConstructorMap::value_type ) {
+return Ctor.second.second == TargetCtor;
+  });
+  if (It == Ctors.end())
+return llvm::None;
+  return std::make_pair(It->first, It->second.first);
+}
+
 std::vector
-Registry::getMatchingMatchers(ast_type_traits::ASTNodeKind StaticType) {
+getDerivedResults(ast_type_traits::ASTNodeKind StaticType, StringRef Name);
+
+enum MatchScope { ExactOnly, IncludeDerived };
+
+std::vector
+getMatchingMatchersImpl(ast_type_traits::ASTNodeKind StaticType,
+MatchScope Scope) {
+
   std::vector Result;
 
   std::vector AcceptedTypes{StaticType};
 
   processAcceptableMatchers(
-  AcceptedTypes,
-  [](StringRef Name, const MatcherDescriptor &,
-std::set &, std::vector>,
-unsigned) { Result.emplace_back((Name + "()").str()); });
+  AcceptedTypes, [&](StringRef Name, const MatcherDescriptor ,
+ std::set &,
+ std::vector>, unsigned) {
+unsigned Specificity;
+ASTNodeKind LeastDerivedKind;
+if (Scope == ExactOnly &&
+Matcher.isConvertibleTo(StaticType, ,
+) &&
+!LeastDerivedKind.isSame(StaticType))
+  return;
+
+auto TypeForMatcherOpt = getNodeConstructorType();
+if (TypeForMatcherOpt &&
+StaticType.isBaseOf(TypeForMatcherOpt->first)) {
+  auto Derived = getDerivedResults(TypeForMatcherOpt->first, Name);
+  Result.insert(Result.end(), Derived.begin(), Derived.end());
+  return;
+}
+
+Result.emplace_back((Name + "()").str());
+  });
+
+  return Result;
+}
+
+std::vector
+getDerivedResults(ast_type_traits::ASTNodeKind StaticType, StringRef Name) {
+  auto NestedResult = getMatchingMatchersImpl(StaticType, ExactOnly);
+
+  std::vector Result;
+
+  Diagnostics DiagnosticIgnorer;
+
+  for (const auto  : NestedResult) {
+Result.emplace_back((Name + "(" + Item.MatcherString + ")").str());
+  }
+
+  return Result;
+}
+
+std::vector
+Registry::getMatchingMatchers(ast_type_traits::ASTNodeKind StaticType) {
+
+  std::vector Result =
+  getMatchingMatchersImpl(StaticType, IncludeDerived);
 
   return Result;
 }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D54408: [ASTMatchers] Add matchers available through casting to derived

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: lib/ASTMatchers/Dynamic/Registry.cpp:651
+llvm::Optional>
+getNodeConstructorType(MatcherCtor targetCtor) {
+  const auto  = RegistryData->nodeConstructors();

targetCtor -> TargetCtor



Comment at: lib/ASTMatchers/Dynamic/Registry.cpp:653
+  const auto  = RegistryData->nodeConstructors();
+  auto it = llvm::find_if(
+  ctors, [targetCtor](const NodeConstructorMap::value_type ) {

it -> It



Comment at: lib/ASTMatchers/Dynamic/Registry.cpp:654
+  auto it = llvm::find_if(
+  ctors, [targetCtor](const NodeConstructorMap::value_type ) {
+return ctor.second.second == targetCtor;

ctor -> Ctor



Comment at: lib/ASTMatchers/Dynamic/Registry.cpp:657-659
+  if (it == ctors.end()) {
+return llvm::None;
+  }

Elide braces



Comment at: lib/ASTMatchers/Dynamic/Registry.cpp:683
+if (Scope == ExactOnly) {
+  if (Matcher.isConvertibleTo(StaticType, ,
+  ) &&

Can combine with the previous `if` statement, I believe.



Comment at: lib/ASTMatchers/Dynamic/Registry.cpp:689
+
+auto TypeForMatcherOpt = getNodeConstructorType();
+if (TypeForMatcherOpt &&

Don't use `auto` here please.



Comment at: lib/ASTMatchers/Dynamic/Registry.cpp:692
+StaticType.isBaseOf(TypeForMatcherOpt->first)) {
+  auto Derived = getDerivedResults(TypeForMatcherOpt->first, Name);
+  Result.insert(Result.end(), Derived.begin(), Derived.end());

Don't use `auto` here please.



Comment at: lib/ASTMatchers/Dynamic/Registry.cpp:705
+getDerivedResults(ast_type_traits::ASTNodeKind StaticType, StringRef Name) {
+  auto NestedResult = getMatchingMatchersImpl(StaticType, ExactOnly);
+

Don't use `auto` here please.



Comment at: lib/ASTMatchers/Dynamic/Registry.cpp:711
+
+  for (const auto  : NestedResult) {
+Result.emplace_back((Name + "(" + item.MatcherString + ")").str());

item -> Item


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D54408/new/

https://reviews.llvm.org/D54408



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

[PATCH] D54408: [ASTMatchers] Add matchers available through casting to derived

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

Differential lacks description.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D54408/new/

https://reviews.llvm.org/D54408



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