Re: [PATCH] D24361: hasDeclaration(qualType(...)) matcher should unwrap ElaboratedType and TemplateSpecializationType

2016-09-17 Thread Łukasz Anforowicz via cfe-commits 
lukasza added a comment.

Richard, could you please take a look?



Comment at: include/clang/ASTMatchers/ASTMatchersInternal.h:750
@@ +749,3 @@
+else if (auto *ET = Node->getAs())
+  return matchesSpecialized(ET->getNamedType(), Finder, Builder);
+else if (auto *TST = Node->getAs())

There is also a question whether I should not only start supporting not only 
qualType(hasDeclaration(...)) when qualType wraps an ElaboratedType node, but 
also start supporting elaboratedType(hasDeclaration(...)).

Snippets of a patch that could achieve this (but then again - I am not sure if 
this is desirable and/or necessary):

+++ include/clang/ASTMatchers/ASTMatchersInternal.h (working copy)
...
+  /// \brief Gets the QualType from ElaboratedType
+  /// and returns whether the inner matches on it.
+  bool matchesSpecialized(const ElaboratedType &Node,
+  ASTMatchFinder *Finder,
+  BoundNodesTreeBuilder *Builder) const {
+return matchesSpecialized(Node.getNamedType(), Finder, Builder);
+  }
+
...
 /// \brief All types that are supported by HasDeclarationMatcher above.
-typedef TypeList HasDeclarationSupportedTypes;


Comment at: include/clang/ASTMatchers/ASTMatchersInternal.h:753
@@ -749,2 +752,3 @@
+  return matchesSpecialized(*TST, Finder, Builder);
 else if (auto *TT = Node->getAs())
   return matchesDecl(TT->getDecl(), Finder, Builder);

I do notice that there are quite a few similar Node->getAs if statements 
here.  They could be replaced with something "clever" (and probably unreadable):

+++ include/clang/ASTMatchers/ASTMatchersInternal.h (working copy)
...
+  /// \brief Terminating case for recursion over template parameters
+  /// performed by matchesQualTypeAsAnyOf template.
+  template 
+  bool matchesQualTypeAsAnyOf(const QualType &Node, ASTMatchFinder *Finder,
+  BoundNodesTreeBuilder *Builder) const {
+return false;
+  }
+
+  /// \brief Returns true if Node->getAs is not null and matches inner
+  /// matcher for any type U listed in template parameters.
+  template ::value, int>::type = 0>
+  bool matchesQualTypeAsAnyOf(const QualType &Node, ASTMatchFinder *Finder,
+  BoundNodesTreeBuilder *Builder) const {
+if (const U *u = Node->getAs())
+  return matchesSpecialized(*u, Finder, Builder);
+
+return matchesQualTypeAsAnyOf(Node, Finder, Builder);
+  }
+
...
-else if (auto *OCIT = Node->getAs())
-  return matchesDecl(OCIT->getDecl(), Finder, Builder);
-else if (auto *UUT = Node->getAs())
-  return matchesDecl(UUT->getDecl(), Finder, Builder);
-else if (auto *ICNT = Node->getAs())
-  return matchesDecl(ICNT->getDecl(), Finder, Builder);
-return false;
+else if (auto *TD = Node->getAsTagDecl())
+  return matchesDecl(TD, Finder, Builder);
+
+return matchesQualTypeAsAnyOf<
+ArrayType, InjectedClassNameType, ObjCInterfaceType, 
TemplateSpecializationType,
+TypedefType, UnresolvedUsingType, void>(
+Node, Finder, Builder);


Comment at: unittests/ASTMatchers/ASTMatchersTraversalTest.cpp:2124
@@ +2123,3 @@
+  // hasDeclaration should see through:
+  // 1. from ElaboratedType (Namespace::MyTemplate) to
+  //TemplateSpecializationType (MyTemplate)

FWIW, I couldn't repro a similar issue (i.e. VarDecl or ParmVarDecl of a type 
with user-provided declaration, but where hasDeclaration doesn't match 
*anything*) for other types (i.e. AutoType or ArrayType) - only for 
ElaboratedType and TemplateSpecializationType.


https://reviews.llvm.org/D24361



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

Re: [PATCH] D24361: hasDeclaration(qualType(...)) matcher should unwrap ElaboratedType and TemplateSpecializationType

2016-09-23 Thread Łukasz Anforowicz via cfe-commits 
lukasza updated the summary for this revision.
lukasza updated this revision to Diff 72350.
lukasza marked an inline comment as done.
lukasza added a comment.

- Added test where both TemplateSpecializationType and TypedefType are present 
and both should match regardless of code order inside 
HasDeclarationMatcher::matchesSpecialized(const QualType &Node...).  Removed 
significance of order inside this matchesSpecialized method.

- Tweaked test proposed in the previous patchset, so that it matches specific 
decls (rather than any decls)

- Added support for matching hasDeclaration(elaboratedType(...)) + test (in the 
test from the previous patchset).


https://reviews.llvm.org/D24361

Files:
  include/clang/ASTMatchers/ASTMatchers.h
  include/clang/ASTMatchers/ASTMatchersInternal.h
  unittests/ASTMatchers/ASTMatchersTraversalTest.cpp

Index: unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
===
--- unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
+++ unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
@@ -2106,5 +2106,56 @@
functionDecl(hasName("bar"));
 }
 
+TEST(HasDeclaration, ElaboratedTypeAndTemplateSpecializationType) {
+  std::string input =
+  "namespace Namespace {\n"
+  "template\n"
+  "class Template {\n"
+  " public:\n"
+  "  void Method() {}\n"
+  "};\n"
+  "}  // namespace Namespace\n"
+  "template \n"
+  "void Function(Namespace::Template param) {\n"
+  "  param.Method();\n"
+  "};\n";
+
+  // Matcher for ::Namespace::Template template decl.
+  auto param_type_decl_matcher = classTemplateDecl(
+  hasName("Template"),
+  hasParent(namespaceDecl(hasName("Namespace"))),
+  has(templateTypeParmDecl(hasName("T";
+
+  // hasDeclaration / qualType-flavour.
+  EXPECT_TRUE(matches(input, parmVarDecl(
+  hasName("param"),
+  hasType(qualType(hasDeclaration(decl(param_type_decl_matcher)));
+
+  // hasDeclaration / elaboratedType-flavour.
+  EXPECT_TRUE(matches(input, parmVarDecl(
+  hasName("param"),
+  hasType(elaboratedType(hasDeclaration(decl(param_type_decl_matcher)));
+}
+
+TEST(HasDeclaration, TypedefTypeAndTemplateSpecializationTypeAreBothHere) {
+  std::string input =
+  "template\n"
+  "class Foo {};\n"
+  "using Bar = Foo;\n"
+  "Bar variable;\n";
+
+  // hasDeclaration should match typedefNameDecl.
+  EXPECT_TRUE(matches(input, varDecl(
+  hasName("variable"),
+  hasType(qualType(hasDeclaration(decl(
+  typeAliasDecl(hasName("Bar");
+
+  // hasDeclaration should match templateSpecializationType
+  EXPECT_TRUE(matches(input, varDecl(
+  hasName("variable"),
+  hasType(qualType(hasDeclaration(decl(
+  classTemplateSpecializationDecl(;
+}
+
 } // namespace ast_matchers
 } // namespace clang
Index: include/clang/ASTMatchers/ASTMatchersInternal.h
===
--- include/clang/ASTMatchers/ASTMatchersInternal.h
+++ include/clang/ASTMatchers/ASTMatchersInternal.h
@@ -744,16 +744,29 @@
 if (Node.isNull())
   return false;
 
-if (auto *TD = Node->getAsTagDecl())
-  return matchesDecl(TD, Finder, Builder);
-else if (auto *TT = Node->getAs())
-  return matchesDecl(TT->getDecl(), Finder, Builder);
 // Do not use getAs instead of the direct dyn_cast.
 // Calling getAs will return the canonical type, but that type does not
 // store a TemplateTypeParmDecl. We *need* the uncanonical type, if it is
 // available, and using dyn_cast ensures that.
-else if (auto *TTP = dyn_cast(Node.getTypePtr()))
+if (auto *TTP = dyn_cast(Node.getTypePtr()))
   return matchesDecl(TTP->getDecl(), Finder, Builder);
+
+// Try if any of desugaring results match
+// (these are not mutually exclusive).
+if (auto *TST = Node->getAs()) {
+  if (matchesSpecialized(*TST, Finder, Builder))
+return true;
+}
+if (auto *TT = Node->getAs()) {
+  if (matchesDecl(TT->getDecl(), Finder, Builder))
+return true;
+}
+
+// Try non-desugarable types.
+if (auto *TD = Node->getAsTagDecl())
+  return matchesDecl(TD, Finder, Builder);
+else if (auto *ET = Node->getAs())
+  return matchesSpecialized(*ET, Finder, Builder);
 else if (auto *OCIT = Node->getAs())
   return matchesDecl(OCIT->getDecl(), Finder, Builder);
 else if (auto *UUT = Node->getAs())
@@ -760,9 +773,18 @@
   return matchesDecl(UUT->getDecl(), Finder, Builder);
 else if (auto *ICNT = Node->getAs())
   return matchesDecl(ICNT->getDecl(), Finder, Builder);
+
 return false;
   }
 
+  /// \brief Gets the QualType from ElaboratedType
+  /// and returns whether the inner matches on it.
+  bool matchesSpecialized(const ElaboratedType &Node,
+  ASTMatchFinder *Finder,
+  BoundNodesTreeBuilder *

Re: [PATCH] D24361: hasDeclaration(qualType(...)) matcher should unwrap ElaboratedType and TemplateSpecializationType

2016-09-23 Thread Łukasz Anforowicz via cfe-commits 
lukasza added inline comments.


Comment at: include/clang/ASTMatchers/ASTMatchersInternal.h:750
@@ +749,3 @@
+else if (auto *ET = Node->getAs())
+  return matchesSpecialized(ET->getNamedType(), Finder, Builder);
+else if (auto *TST = Node->getAs())

lukasza wrote:
> There is also a question whether I should not only start supporting not only 
> qualType(hasDeclaration(...)) when qualType wraps an ElaboratedType node, but 
> also start supporting elaboratedType(hasDeclaration(...)).
> 
> Snippets of a patch that could achieve this (but then again - I am not sure 
> if this is desirable and/or necessary):
> 
> +++ include/clang/ASTMatchers/ASTMatchersInternal.h   (working copy)
> ...
> +  /// \brief Gets the QualType from ElaboratedType
> +  /// and returns whether the inner matches on it.
> +  bool matchesSpecialized(const ElaboratedType &Node,
> +  ASTMatchFinder *Finder,
> +  BoundNodesTreeBuilder *Builder) const {
> +return matchesSpecialized(Node.getNamedType(), Finder, Builder);
> +  }
> +
> ...
>  /// \brief All types that are supported by HasDeclarationMatcher above.
> -typedef TypeList - InjectedClassNameType, LabelStmt, AddrLabelExpr, 
> MemberExpr,
> - QualType, RecordType, TagType, 
> TemplateSpecializationType,
> - TemplateTypeParmType, TypedefType,
> +typedef TypeList ElaboratedType,
> + EnumType, InjectedClassNameType, LabelStmt, 
> AddrLabelExpr,
> + MemberExpr, QualType, RecordType, TagType,
> + TemplateSpecializationType, TemplateTypeParmType, 
> TypedefType,
>   UnresolvedUsingType> HasDeclarationSupportedTypes;
FWIW, I've added this support in the latest patchset.


Comment at: include/clang/ASTMatchers/ASTMatchersInternal.h:752
@@ -749,1 +751,3 @@
+else if (auto *TST = Node->getAs())
+  return matchesSpecialized(*TST, Finder, Builder);
 else if (auto *TT = Node->getAs())

rsmith wrote:
> lukasza wrote:
> > I am a little bit confused and concerned whether the order of the if 
> > statements here matters.  In particular - I am not sure if the ordering of 
> > dyn_cast below and getAs calls here matters (and whether things would be 
> > safer / less fragile if the dyn_cast was moved all the way to the top?).
> I don't think the position of the `TemplateTypeParmType` case matters; a 
> non-canonical TTPT always desugars to a canonical TTPT, so none of the other 
> cases can match.
> 
> The relative order of the `getAs` cases can matter, though, and neither 
> ordering of `TypedefType` and `TemplateSpecializationType` will actually do 
> the right thing in general (you can have a typedef that desugars to a TST and 
> vice versa -- the TST would need to represent an alias template 
> specialization in the latter case). If you want to get this "right" you'll 
> need to do single-step desugaring until you hit a type you like the look of. 
> (See `getAsSugar` in lib/AST/Type.cpp for an example.)
Thank you for the explanation and the hint to look at getAsSugar.

I've added a test where both TypeSpecializationType and TypedefType could be 
used.  One of the asserts (the one trying to match typeAliasDecl) was initially 
failing as expected / as explained by your comment above (i.e. 
HasDeclarationMatcher::matchesSpecialized(QualType...) was first looking at 
TypeSpecializationType and when that succeeded it would not consider 
TypedefType later on).  The test covers 
alias-desugars-to-template-specialization-type case.

Unfortunately, I was not able to come up with desugaring going in the other 
direction.  I started with something like what I have below, but there is no 
typeAliasDecl here (I think) and I don't know how to insert one:

  template
  class Foo {};
  template 
  using Bar = Foo;
  Bar variable;

I am not sure if I really need to do single-step desugaring (and consult 
hasDeclaration's innerMatcher for each step).  Wouldn't it be sufficient that 
desugaring to TemplateSpecializationType does not latch onto TST code path, but 
that we will also consider TypedefType desugaring later?  Code is probably 
better at explaining at what I mean here - please look at the latest patchset.  
The new test passes, so I think my changes really made ordering insignificant 
as desired (at least insignificant when it comes to ordering between the only 
types (TemplateSpecializationType and TypedefType) that are 1) used in 
HasDeclarationMatcher / QualType overload + 2) covered via getAsSugar helper).


Comment at: unittests/ASTMatchers/ASTMatchersTraversalTest.cpp:2119
@@ +2118,3 @@
+  "template \n"
+  "void Function(Namespace::Template param) {\n"
+  "  param.Method();\n"

rsmith wrote:
> @klimek, would y

Re: [PATCH] D24361: hasDeclaration(qualType(...)) matcher should unwrap ElaboratedType and TemplateSpecializationType

2016-09-26 Thread Łukasz Anforowicz via cfe-commits 
lukasza added inline comments.


Comment at: unittests/ASTMatchers/ASTMatchersTraversalTest.cpp:2119
@@ +2118,3 @@
+  "template \n"
+  "void Function(Namespace::Template param) {\n"
+  "  param.Method();\n"

klimek wrote:
> Given your use case: why do we need hasDeclaration here at all?
> I'd have expected this working with just matching on the nested name 
> specifier of the type instead of saying hasDeclaration on the template type.
> Btw, if you add a type alias for a class not in the namespace into the 
> namespace (typedef / using), do you wan that to rename or not? :)
> 
> I'd personally probably have expected (2), but I'm never sure in these cases 
> without playing around with more test cases...
> Given your use case: why do we need hasDeclaration here at all?
> I'd have expected this working with just matching on the nested name 
> specifier of the type instead of saying hasDeclaration on the template type.

Because I want "namespace-of-user-provided-declaration" matching to work both 
for ElaboratedType nodes (with explicit nested name specifier) and for other 
kinds of nodes (where there might be no nested name specifier).  I was hoping 
that I could do this with a single hasDeclaration matcher, rather than listing 
all possible type nodes myself (when building my own matcher) like I sort of do 
in a workaround.  In particular, after this CL a single, simple 
hasDeclaration-based matcher can be used in
//auto blink_qual_type_base_matcher =
//qualType(hasDeclaration(in_blink_namespace));
inside https://codereview.chromium.org/2256913002/patch/180001/190001.

> Btw, if you add a type alias for a class not in the namespace into the 
> namespace (typedef / using), do you wan that to rename or not? :)

Good question.  I want a rename to happen if I have 
::SomeOtherNamespace::Typedef resolving to 
::NamespaceWithRenamedMethods::Class, but I do not want rename to happen if I 
have ::NamespaceWithRenamtedMethods::Typedef resolving to 
::SomeOtherNamespace::Class.  I guess my current hasDeclaration-based matcher 
will match both cases :-(  One way to fix this would be to exclude typedefs in 
|decl_under_blink_namespace| at 
https://chromium.googlesource.com/chromium/src/+/14d095b4df6754fa4e6959220b2b332db0b4f504/tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp#646

But... this question+answer should have no impact on the CL under review, right?

> I'd personally probably have expected (2), but I'm never sure in these cases 
> without playing around with more test cases...

Ok.  This (#2) is what the current patch results in.


https://reviews.llvm.org/D24361



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

[PATCH] D24361: hasDeclaration(qualType(...)) matcher should unwrap ElaboratedType and TemplateSpecializationType

2016-10-04 Thread Łukasz Anforowicz via cfe-commits 
lukasza added a comment.

Richard - what are the next steps for this patch?


https://reviews.llvm.org/D24361



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

Re: [PATCH] D24268: Traversing template paramter lists of DeclaratorDecls and/or TagDecls.

2016-09-06 Thread Łukasz Anforowicz via cfe-commits 
lukasza updated the summary for this revision.
lukasza updated this revision to Diff 70483.
lukasza added a comment.

Addressing CR feedbackfrom rsmith@.


https://reviews.llvm.org/D24268

Files:
  include/clang/AST/RecursiveASTVisitor.h
  unittests/ASTMatchers/ASTMatchersTraversalTest.cpp

Index: unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
===
--- unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
+++ unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
@@ -641,6 +641,89 @@
 functionDecl(hasAnyTemplateArgument(templateArgument();
 }
 
+TEST(TemplateTypeParmDecl, CXXMethodDecl) {
+  const char input[] =
+  "template\n"
+  "class Class {\n"
+  "  void method();\n"
+  "};\n"
+  "template\n"
+  "void Class::method() {}\n";
+  EXPECT_TRUE(matches(input, templateTypeParmDecl(hasName("T";
+  EXPECT_TRUE(matches(input, templateTypeParmDecl(hasName("U";
+}
+
+TEST(TemplateTypeParmDecl, VarDecl) {
+  const char input[] =
+  "template\n"
+  "class Class {\n"
+  "  static T pi;\n"
+  "};\n"
+  "template\n"
+  "U Class::pi = U(3.1415926535897932385);\n";
+  EXPECT_TRUE(matches(input, templateTypeParmDecl(hasName("T";
+  EXPECT_TRUE(matches(input, templateTypeParmDecl(hasName("U";
+}
+
+TEST(TemplateTypeParmDecl, VarTemplatePartialSpecializationDecl) {
+  const char input[] =
+  "template\n"
+  "struct Struct {\n"
+  "  template static int field;\n"
+  "};\n"
+  "template\n"
+  "template\n"
+  "int Struct::field = 123;\n";
+  EXPECT_TRUE(matches(input, templateTypeParmDecl(hasName("T";
+  EXPECT_TRUE(matches(input, templateTypeParmDecl(hasName("T2";
+  EXPECT_TRUE(matches(input, templateTypeParmDecl(hasName("U";
+  EXPECT_TRUE(matches(input, templateTypeParmDecl(hasName("U2";
+}
+
+TEST(TemplateTypeParmDecl, ClassTemplatePartialSpecializationDecl) {
+  const char input[] =
+  "template\n"
+  "class Class {\n"
+  "  template struct Struct;\n"
+  "};\n"
+  "template\n"
+  "template\n"
+  "struct Class::Struct {};\n";
+  EXPECT_TRUE(matches(input, templateTypeParmDecl(hasName("T";
+  EXPECT_TRUE(matches(input, templateTypeParmDecl(hasName("T2";
+  EXPECT_TRUE(matches(input, templateTypeParmDecl(hasName("U";
+  EXPECT_TRUE(matches(input, templateTypeParmDecl(hasName("U2";
+}
+
+TEST(TemplateTypeParmDecl, EnumDecl) {
+  const char input[] =
+  "template\n"
+  "struct Struct {\n"
+  "  enum class Enum : T;\n"
+  "};\n"
+  "template\n"
+  "enum class Struct::Enum : U {\n"
+  "  e1,\n"
+  "  e2\n"
+  "};\n";
+  EXPECT_TRUE(matches(input, templateTypeParmDecl(hasName("T";
+  EXPECT_TRUE(matches(input, templateTypeParmDecl(hasName("U";
+}
+
+TEST(TemplateTypeParmDecl, RecordDecl) {
+  const char input[] =
+  "template\n"
+  "class Class {\n"
+  "  struct Struct;\n"
+  "};\n"
+  "template\n"
+  "struct Class::Struct {\n"
+  "  U field;\n"
+  "};\n";
+  EXPECT_TRUE(matches(input, templateTypeParmDecl(hasName("T";
+  EXPECT_TRUE(matches(input, templateTypeParmDecl(hasName("U";
+}
+
 TEST(RefersToIntegralType, Matches) {
   EXPECT_TRUE(matches("template struct C {}; C<42> c;",
   classTemplateSpecializationDecl(
Index: include/clang/AST/RecursiveASTVisitor.h
===
--- include/clang/AST/RecursiveASTVisitor.h
+++ include/clang/AST/RecursiveASTVisitor.h
@@ -482,6 +482,11 @@
 private:
   // These are helper methods used by more than one Traverse* method.
   bool TraverseTemplateParameterListHelper(TemplateParameterList *TPL);
+
+  // Traverses template parameter lists of either a DeclaratorDecl or TagDecl.
+  template 
+  bool TraverseDeclTemplateParameterLists(T *D);
+
 #define DEF_TRAVERSE_TMPL_INST(TMPLDECLKIND)   \
   bool TraverseTemplateInstantiations(TMPLDECLKIND##TemplateDecl *D);
   DEF_TRAVERSE_TMPL_INST(Class)
@@ -1532,6 +1537,16 @@
 }
 
 template 
+template 
+bool RecursiveASTVisitor::TraverseDeclTemplateParameterLists(T *D) {
+  for (unsigned i = 0; i < D->getNumTemplateParameterLists(); i++) {
+TemplateParameterList *TPL = D->getTemplateParameterList(i);
+TraverseTemplateParameterListHelper(TPL);
+  }
+  return true;
+}
+
+template 
 bool RecursiveASTVisitor::TraverseTemplateInstantiations(
 ClassTemplateDecl *D) {
   for (auto *SD : D->specializations()) {
@@ -1692,6 +1707,8 @@
 })
 
 DEF_TRAVERSE_DECL(EnumDecl, {
+  TRY_TO(TraverseDeclTemplateParameterLists(D));
+
   if (D->getTypeForDecl())
 TRY_TO(TraverseType(QualType(D->getTypeForDecl(), 0)));
 
@@ -1706,6 +1723,7 @@
   // We shouldn't traverse D->getTypeForDecl(); it's a result of
   // declaring the type, not something that was written in the source.
 
+  TRY_TO(TraverseDeclTemplateParameterLists(D));
   TRY_TO(TraverseNestedNameS

[PATCH] D24268: Traversing template paramter lists of DeclaratorDecls and/or TagDecls.

2016-09-06 Thread Łukasz Anforowicz via cfe-commits 
lukasza created this revision.
lukasza added a reviewer: rsmith.
lukasza added a subscriber: cfe-commits.
Herald added a subscriber: klimek.

The unit tests in this patch demonstrate the need to traverse template
parameter lists of DeclaratorDecls (e.g. VarDecls, CXXMethodDecls) and
TagDecls (e.g. EnumDecls, RecordDecls).  Without new traversal code
in RecursiveASTVisitor, the new unit tests would trigger an assert
failure in MatchASTVisitor::matchesAncestorOfRecursively while verifing
that all AST nodes have a parent:
  bool matchesAncestorOfRecursively(const ast_type_traits::DynTypedNode &Node,
...) {
const auto &Parents = ActiveASTContext->getParents(Node);
assert(!Parents.empty() && "Found node that is not in the parent map.");

Fixes PR29042.

https://reviews.llvm.org/D24268

Files:
  include/clang/AST/RecursiveASTVisitor.h
  unittests/ASTMatchers/ASTMatchersTraversalTest.cpp

Index: unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
===
--- unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
+++ unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
@@ -1720,6 +1720,68 @@
 "}\n",
   declRefExpr(to(decl(hasAncestor(decl()));
 }
+
+AST_MATCHER_P(QualType,
+  hasUnderlyingType,
+  internal::Matcher,
+  InnerMatcher) {
+  const Type* type = Node.getTypePtrOrNull();
+  return type && InnerMatcher.matches(*type, Finder, Builder);
+}
+
+TEST(HasAncestor, TemplateTypeParmDeclUnderCXXMethodDecl) {
+  EXPECT_TRUE(matches(
+  "template\n"
+  "class Class {\n"
+  "  void method();\n"
+  "};\n"
+  "template\n"
+  "void Class::method() {}\n",
+  qualType(hasUnderlyingType(templateTypeParmType(hasDeclaration(decl(
+  hasAncestor(decl();
+}
+
+TEST(HasAncestor, TemplateTypeParmDeclUnderVarDecl) {
+  EXPECT_TRUE(matches(
+  "template\n"
+  "class Class {\n"
+  "  static T pi;\n"
+  "};\n"
+  "template\n"
+  "U Class::pi = U(3.1415926535897932385);\n",
+  qualType(hasUnderlyingType(templateTypeParmType(hasDeclaration(decl(
+  hasAncestor(decl();
+}
+
+TEST(HasAncestor, TemplateTypeParmDeclUnderEnumDecl) {
+  EXPECT_TRUE(matches(
+  "template\n"
+  "struct Struct {\n"
+  "  enum class Enum : T;\n"
+  "};\n"
+  "template\n"
+  "enum class Struct::Enum : U {\n"
+  "  e1,\n"
+  "  e2\n"
+  "};\n",
+  qualType(hasUnderlyingType(templateTypeParmType(hasDeclaration(decl(
+  hasAncestor(decl();
+}
+
+TEST(HasAncestor, TemplateTypeParmDeclUnderStructDecl) {
+  EXPECT_TRUE(matches(
+  "template\n"
+  "class Class {\n"
+  "  struct Struct;\n"
+  "};\n"
+  "template\n"
+  "struct Class::Struct {\n"
+  "  U field;\n"
+  "};\n",
+  qualType(hasUnderlyingType(templateTypeParmType(hasDeclaration(decl(
+  hasAncestor(decl();
+}
+
 TEST(HasAncestor, NonParmDependentTemplateParmVarDeclRefExpr) {
   EXPECT_TRUE(matches("struct PartitionAllocator {\n"
 "  template\n"
Index: include/clang/AST/RecursiveASTVisitor.h
===
--- include/clang/AST/RecursiveASTVisitor.h
+++ include/clang/AST/RecursiveASTVisitor.h
@@ -482,6 +482,13 @@
 private:
   // These are helper methods used by more than one Traverse* method.
   bool TraverseTemplateParameterListHelper(TemplateParameterList *TPL);
+
+  // Traverses template parameter lists of either a DeclaratorDecl or TagDecl.
+  template ::value ||
+  std::is_base_of::value>::type>
+  bool TraverseDeclTemplateParameterLists(T *D);
+
 #define DEF_TRAVERSE_TMPL_INST(TMPLDECLKIND)   \
   bool TraverseTemplateInstantiations(TMPLDECLKIND##TemplateDecl *D);
   DEF_TRAVERSE_TMPL_INST(Class)
@@ -1532,6 +1539,16 @@
 }
 
 template 
+template 
+bool RecursiveASTVisitor::TraverseDeclTemplateParameterLists(T *D) {
+  for (unsigned i = 0; i < D->getNumTemplateParameterLists(); i++) {
+TemplateParameterList *TPL = D->getTemplateParameterList(i);
+TraverseTemplateParameterListHelper(TPL);
+  }
+  return true;
+}
+
+template 
 bool RecursiveASTVisitor::TraverseTemplateInstantiations(
 ClassTemplateDecl *D) {
   for (auto *SD : D->specializations()) {
@@ -1692,6 +1709,8 @@
 })
 
 DEF_TRAVERSE_DECL(EnumDecl, {
+  TRY_TO(TraverseDeclTemplateParameterLists(D));
+
   if (D->getTypeForDecl())
 TRY_TO(TraverseType(QualType(D->getTypeForDecl(), 0)));
 
@@ -1706,6 +1725,7 @@
   // We shouldn't traverse D->getTypeForDecl(); it's a result of
   // declaring the type, not something that was written in the source.
 
+  TRY_TO(TraverseDeclTemplateParameterLists(D));
   TRY_TO(TraverseNestedNameSpecifierLoc(D->getQualifierLoc()));
   return true;
 }
@@ -1800,6 +1820,7 @@
 
 template 
 bool RecursiveASTVisitor::TraverseDeclarato

Re: [PATCH] D24268: Traversing template paramter lists of DeclaratorDecls and/or TagDecls.

2016-09-06 Thread Łukasz Anforowicz via cfe-commits 
lukasza added inline comments.


Comment at: include/clang/AST/RecursiveASTVisitor.h:487-489
@@ +486,5 @@
+  // Traverses template parameter lists of either a DeclaratorDecl or TagDecl.
+  template ::value ||
+  std::is_base_of::value>::type>
+  bool TraverseDeclTemplateParameterLists(T *D);

rsmith wrote:
> What's the purpose of the `enable_if` here?
enable_if is here to document via code that this method works for arguments 
that are either a TagDecl or a DeclaratorDecl.  enable_if is not really 
necessary here - I could take it out and just make the 
TraverseDeclTemplateParameterLists method compile as long as T contains the 
right getNumTemplateParameterLists and getTemplateParameterList methods.

FWIW, I got rid of std::enable_if above in the latest patch revision.  Does the 
code look better and less surprising now?


Comment at: include/clang/AST/RecursiveASTVisitor.h:1728
@@ -1708,2 +1727,3 @@
 
+  TRY_TO(TraverseDeclTemplateParameterLists(D));
   TRY_TO(TraverseNestedNameSpecifierLoc(D->getQualifierLoc()));

rsmith wrote:
> lukasza wrote:
> > I think that tweaks of EnumDecl traversal together with tweaks of 
> > TraverseRecordHelper tweaks, take care of all the relevant node types 
> > derived from TagDecl:
> > 
> > - EnumDecl - traversal now calls TraverseDeclTemplateParameterLists 
> > (+EnumDecl is covered by the new unit tests)
> > - RecordDecl - TraverseRecordHelper now calls 
> > TraverseDeclTemplateParameterLists (+RecordDecl is covered by the new unit 
> > tests)
> > - CXXRecordDecl - traversal includes a call to TraverseRecordHelper
> > 
> > I believe that nodes derived further from CXXRecordDecl cannot have a 
> > template parameter list (?):
> > - ClassTemplateSpecializationDecl
> > - ClassTemplatePartialSpecializationDecl
> > If this is not correct, then can you please help me see the shape of unit 
> > tests that would cover these nodes?
> I don't think `ClassTemplateSpecializationDecl` can have a template parameter 
> list for its qualifier. A `ClassTemplatePartialSpecializationDecl` can, 
> though:
> 
>   template struct A {
> template struct B; 
>   };
>   template template struct A::B {};
Thanks for the example.  I've added a unit test covering this case (and 
verified that it fails before the fix and passes afterwards).  A bit 
surprisingly, the extra test passed without further tweaks to the product code 
(surprisingly, because I am not sure how TraverseDeclTemplateParameterLists 
ends up getting called for ClassTemplatePartialSpecializationDecl).  I don't 
think I want to keep digging to understand why things work without further 
tweaks, but the surprise factor makes me think the extra test is worth keeping 
(even though it apparently doesn't really increase test coverage).


Comment at: include/clang/AST/RecursiveASTVisitor.h:1823
@@ -1802,2 +1822,3 @@
 bool RecursiveASTVisitor::TraverseDeclaratorHelper(DeclaratorDecl *D) 
{
+  TRY_TO(TraverseDeclTemplateParameterLists(D));
   TRY_TO(TraverseNestedNameSpecifierLoc(D->getQualifierLoc()));

rsmith wrote:
> lukasza wrote:
> > I think that TraverseDeclaratorHelper together with TraverseFunctionHelper 
> > tweaks, this takes care of all node types derived from DeclaratorDecl:
> > - FieldDecl - traversal calls TraverseDeclaratorHelper
> > - FunctionDecl - traversal calls TraverseFunctionHelper
> > - CXXMethodDecl - traversal calls TraverseFunctionHelper (+CXXMethodDecl is 
> > covered by the new unit tests)
> > - CXXConstructorDecl - traversal calls TraverseFunctionHelper
> > - CXXConversionDecl - traversal calls TraverseFunctionHelper
> > - CXXDestructorDecl - traversal calls TraverseFunctionHelper
> > - VarDecl - TraverseVarHelper calls TraverseDeclaratorHelper (+VarDecl is 
> > covered by the new unit tests)
> > 
> > I believe that nodes derived further from VarDecl cannot have a template 
> > parameter list (?), but most of them should still be safe (because their 
> > traversal goes through TraverseVarHelper):
> > - DecompositionDecl - traversal calls TraverseVarHelper
> > - ImplicitParamDecl - traversal calls TraverseVarHelper
> > - OMPCapturedExprDecl - traversal calls TraverseVarHelper
> > - ParmVarDecl - traversal calls TraverseVarHelper
> > - VarTemplateSpecializationDecl
> > - VarTemplatePartialSpecializationDecl
> > 
> > Please let me know if I am wrong above and it is indeed possible to have 
> > template parameter lists next to VarTemplateSpecializationDecl and/or 
> > VarTemplatePartialSpecializationDecl (and I would also appreciate if you 
> > could help me see the shape of unit tests that would cover these nodes).
> `VarTemplatePartialSpecializationDecl`s can have template parameter lists:
> 
>   template struct A { template static int n; };
>   template template int A::n;
> 
> I don't think other `VarTemplateSpecializationDecl`s can.
Thanks for the example.  I've added a unit test covering this case (and 
verifi

Re: [PATCH] D24268: Traversing template paramter lists of DeclaratorDecls and/or TagDecls.

2016-09-06 Thread Łukasz Anforowicz via cfe-commits 
lukasza added inline comments.


Comment at: include/clang/AST/RecursiveASTVisitor.h:1728
@@ -1708,2 +1727,3 @@
 
+  TRY_TO(TraverseDeclTemplateParameterLists(D));
   TRY_TO(TraverseNestedNameSpecifierLoc(D->getQualifierLoc()));

I think that tweaks of EnumDecl traversal together with tweaks of 
TraverseRecordHelper tweaks, take care of all the relevant node types derived 
from TagDecl:

- EnumDecl - traversal now calls TraverseDeclTemplateParameterLists (+EnumDecl 
is covered by the new unit tests)
- RecordDecl - TraverseRecordHelper now calls 
TraverseDeclTemplateParameterLists (+RecordDecl is covered by the new unit 
tests)
- CXXRecordDecl - traversal includes a call to TraverseRecordHelper

I believe that nodes derived further from CXXRecordDecl cannot have a template 
parameter list (?):
- ClassTemplateSpecializationDecl
- ClassTemplatePartialSpecializationDecl
If this is not correct, then can you please help me see the shape of unit tests 
that would cover these nodes?


Comment at: include/clang/AST/RecursiveASTVisitor.h:1823
@@ -1802,2 +1822,3 @@
 bool RecursiveASTVisitor::TraverseDeclaratorHelper(DeclaratorDecl *D) 
{
+  TRY_TO(TraverseDeclTemplateParameterLists(D));
   TRY_TO(TraverseNestedNameSpecifierLoc(D->getQualifierLoc()));

I think that TraverseDeclaratorHelper together with TraverseFunctionHelper 
tweaks, this takes care of all node types derived from DeclaratorDecl:
- FieldDecl - traversal calls TraverseDeclaratorHelper
- FunctionDecl - traversal calls TraverseFunctionHelper
- CXXMethodDecl - traversal calls TraverseFunctionHelper (+CXXMethodDecl is 
covered by the new unit tests)
- CXXConstructorDecl - traversal calls TraverseFunctionHelper
- CXXConversionDecl - traversal calls TraverseFunctionHelper
- CXXDestructorDecl - traversal calls TraverseFunctionHelper
- VarDecl - TraverseVarHelper calls TraverseDeclaratorHelper (+VarDecl is 
covered by the new unit tests)

I believe that nodes derived further from VarDecl cannot have a template 
parameter list (?), but most of them should still be safe (because their 
traversal goes through TraverseVarHelper):
- DecompositionDecl - traversal calls TraverseVarHelper
- ImplicitParamDecl - traversal calls TraverseVarHelper
- OMPCapturedExprDecl - traversal calls TraverseVarHelper
- ParmVarDecl - traversal calls TraverseVarHelper
- VarTemplateSpecializationDecl
- VarTemplatePartialSpecializationDecl

Please let me know if I am wrong above and it is indeed possible to have 
template parameter lists next to VarTemplateSpecializationDecl and/or 
VarTemplatePartialSpecializationDecl (and I would also appreciate if you could 
help me see the shape of unit tests that would cover these nodes).


Comment at: include/clang/AST/RecursiveASTVisitor.h:1870
@@ -1848,2 +1869,3 @@
 bool RecursiveASTVisitor::TraverseFunctionHelper(FunctionDecl *D) {
+  TRY_TO(TraverseDeclTemplateParameterLists(D));
   TRY_TO(TraverseNestedNameSpecifierLoc(D->getQualifierLoc()));

Richard, in https://llvm.org/bugs/show_bug.cgi?id=29042#c6 you've suggested 
reusing TraverseDeclaratorHelper here.  I agree that there are refactoring and 
code reusing opportunities here (e.g. TraverseNestedNameSpecifier[Loc] are 
getting called from both TraverseDeclaratorHelper and from 
TraverseFunctionHelper), but I didn't pursue them in this patch, because I was 
worried about little details:

1. traversal order (i.e. TraverseDeclaratorHelper traverses NestedNameSpecifier 
immediately before traversing TypeSourceInfo;  this is different than 
TraverseFunctionHelper which traverses DeclarationNameInfo in-between 
traversing NestedNameSpecifier and TypeSourceInfo)

2. traversal in absence of TypeSourceInfo (i.e. when TypeSourceInfo is missing, 
then TraverseDeclaratorHelper traverses Type;  this is different than 
TraverseFunctionHelper which in this case 1) checks shouldVisitImplicitCode 
condition and only then 2) traverses function parameters).

Because of the above, I think that for now it is better to call 
TraverseDeclTemplateParameterLists directly from TraverseFunctionHelper, rather 
than trying to reuse the whole (or bigger part of) TraverseDeclaratorHelper 
from TraverseFunctionHelper.


Comment at: unittests/ASTMatchers/ASTMatchersTraversalTest.cpp:1730
@@ +1729,3 @@
+  return type && InnerMatcher.matches(*type, Finder, Builder);
+}
+

I've needed this custom AST matcher in 
https://codereview.chromium.org/2256913002 - let me try to sneak in a question 
related to this other CL:  Can I somehow avoid introducing a new, custom AST 
matcher?  (i.e. is it possible to express hasUnderlyingType in terms of 
built-in matchers?)


https://reviews.llvm.org/D24268



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

Re: [PATCH] D24268: Traversing template paramter lists of DeclaratorDecls and/or TagDecls.

2016-09-07 Thread Łukasz Anforowicz via cfe-commits 
lukasza added a comment.

In https://reviews.llvm.org/D24268#535463, @rsmith wrote:

> This patch looks great, thank you! Do you have an SVN account or do you need 
> someone to commit this for you?


I don't have an SVN account.  Could you please commit the patch for me?


https://reviews.llvm.org/D24268



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

[PATCH] D24361: hasDeclaration(qualType(...)) matcher should unwrap ElaboratedType and TemplateSpecializationType

2016-10-25 Thread Łukasz Anforowicz via cfe-commits 
lukasza updated the summary for this revision.
lukasza updated this revision to Diff 75661.
lukasza added a comment.

Reverted changes in the patch that are not related to the issue of 
hasDeclaration not matching *anything* in some cases.


https://reviews.llvm.org/D24361

Files:
  include/clang/ASTMatchers/ASTMatchers.h
  include/clang/ASTMatchers/ASTMatchersInternal.h
  unittests/ASTMatchers/ASTMatchersTraversalTest.cpp


Index: unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
===
--- unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
+++ unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
@@ -2106,5 +2106,36 @@
functionDecl(hasName("bar"));
 }
 
+TEST(HasDeclaration, ElaboratedTypeAndTemplateSpecializationType) {
+  std::string input =
+  "namespace Namespace {\n"
+  "template\n"
+  "class Template {\n"
+  " public:\n"
+  "  void Method() {}\n"
+  "};\n"
+  "}  // namespace Namespace\n"
+  "template \n"
+  "void Function(Namespace::Template param) {\n"
+  "  param.Method();\n"
+  "};\n";
+
+  // Matcher for ::Namespace::Template template decl.
+  auto param_type_decl_matcher = classTemplateDecl(
+  hasName("Template"),
+  hasParent(namespaceDecl(hasName("Namespace"))),
+  has(templateTypeParmDecl(hasName("T";
+
+  // hasDeclaration / qualType-flavour.
+  EXPECT_TRUE(matches(input, parmVarDecl(
+  hasName("param"),
+  hasType(qualType(hasDeclaration(decl(param_type_decl_matcher)));
+
+  // hasDeclaration / elaboratedType-flavour.
+  EXPECT_TRUE(matches(input, parmVarDecl(
+  hasName("param"),
+  
hasType(elaboratedType(hasDeclaration(decl(param_type_decl_matcher)));
+}
+
 } // namespace ast_matchers
 } // namespace clang
Index: include/clang/ASTMatchers/ASTMatchersInternal.h
===
--- include/clang/ASTMatchers/ASTMatchersInternal.h
+++ include/clang/ASTMatchers/ASTMatchersInternal.h
@@ -748,6 +748,10 @@
   return matchesDecl(TD, Finder, Builder);
 else if (auto *TT = Node->getAs())
   return matchesDecl(TT->getDecl(), Finder, Builder);
+else if (auto *ET = Node->getAs())
+  return matchesSpecialized(ET->getNamedType(), Finder, Builder);
+else if (auto *TST = Node->getAs())
+  return matchesSpecialized(*TST, Finder, Builder);
 // Do not use getAs instead of the direct dyn_cast.
 // Calling getAs will return the canonical type, but that type does not
 // store a TemplateTypeParmDecl. We *need* the uncanonical type, if it is
@@ -763,6 +767,14 @@
 return false;
   }
 
+  /// \brief Gets the QualType from ElaboratedType
+  /// and returns whether the inner matches on it.
+  bool matchesSpecialized(const ElaboratedType &Node,
+  ASTMatchFinder *Finder,
+  BoundNodesTreeBuilder *Builder) const {
+return matchesSpecialized(Node.getNamedType(), Finder, Builder);
+  }
+
   /// \brief Gets the TemplateDecl from a TemplateSpecializationType
   /// and returns whether the inner matches on it.
   bool matchesSpecialized(const TemplateSpecializationType &Node,
@@ -1007,10 +1019,10 @@
  TypeLoc, QualType> AdaptativeDefaultToTypes;
 
 /// \brief All types that are supported by HasDeclarationMatcher above.
-typedef TypeList HasDeclarationSupportedTypes;
 
 /// \brief Converts a \c Matcher to a matcher of desired type \c To by
Index: include/clang/ASTMatchers/ASTMatchers.h
===
--- include/clang/ASTMatchers/ASTMatchers.h
+++ include/clang/ASTMatchers/ASTMatchers.h
@@ -2454,7 +2454,8 @@
 /// function. e.g. various subtypes of clang::Type and various expressions.
 ///
 /// Usable as: Matcher, Matcher,
-///   Matcher, Matcher, Matcher,
+///   Matcher, Matcher, Matcher,
+///   Matcher,
 ///   Matcher, Matcher, Matcher,
 ///   Matcher, Matcher, Matcher,
 ///   Matcher, Matcher,


Index: unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
===
--- unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
+++ unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
@@ -2106,5 +2106,36 @@
functionDecl(hasName("bar"));
 }
 
+TEST(HasDeclaration, ElaboratedTypeAndTemplateSpecializationType) {
+  std::string input =
+  "namespace Namespace {\n"
+  "template\n"
+  "class Template {\n"
+  " public:\n"
+  "  void Method() {}\n"
+  "};\n"
+  "}  // namespace Namespace\n"
+  "template \n"
+  "void Function(Namespace::Template param) {\n"
+  "  param.Method();\n"
+  "};\n";
+
+  // Matcher for ::Namespace::Template template decl.
+  auto param_type_decl_matcher = classTemplateDecl(
+  hasName("Template"),
+  hasParent(namespaceDecl(hasName("Namespace"))),
+  has(template

[PATCH] D26032: [ASTMatcher] Add CXXNewExpr support to hasDeclaration

2016-10-31 Thread Łukasz Anforowicz via cfe-commits 
lukasza added a comment.

FWIW, a non-owner LGTM:

- CXXNewExpr seems very similar to CallExpr, so it makes sense that 
hasDeclaration would behave similarily for both of these expressions (i.e. 
matching the "callee")
- The issues we've been trying to work through in 
https://reviews.llvm.org/D24361 mainly revolve around Type and QualType, so I 
think those issues should not apply to CXXNewExpr matching.


https://reviews.llvm.org/D26032



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