[PATCH] D90767: Add new matchers for dependent names in templates

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:2838
 
+/// Matches template-dependent, but known, member names
+///
----------------
Missing full stop at the end of the comment.


================
Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:2859
+AST_MATCHER_P(CXXDependentScopeMemberExpr, hasMemberName, std::string, N) {
+  return Node.getMember().getAsString() == N;
+}
----------------
This will allow users to match on members that don't have identifiers -- is that intentional? If not, my recommendation is to use something like:
```
if (const IdentifierInfo *II = Node.getMember().getAsIdentifierInfo())
  return II->isStr(N);
return false;
```
Either way, we should document and test what the expected behavior is for things like constructors/destructors, overloaded operators, and the likes. (But we don't have to test every kind of odd declaration name.)


================
Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:2898
+              std::string, BindingID) {
+  auto MemberName = Node.getMember().getAsString();
+
----------------
Similar concerns here about matching members without identifiers.


================
Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:2902
+                                  MemberName](const BoundNodesMap &Nodes) {
+    auto BN = Nodes.getNode(this->BindingID);
+    if (auto ND = BN.get<NamedDecl>()) {
----------------
I assume this is a `const auto &` and not a value type? If so, please update the type.


================
Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:2903
+    auto BN = Nodes.getNode(this->BindingID);
+    if (auto ND = BN.get<NamedDecl>()) {
+      if (!(isa<FieldDecl>(ND) || isa<CXXMethodDecl>(ND) || isa<VarDecl>(ND)))
----------------
`const auto *`


================
Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:2904
+    if (auto ND = BN.get<NamedDecl>()) {
+      if (!(isa<FieldDecl>(ND) || isa<CXXMethodDecl>(ND) || isa<VarDecl>(ND)))
+        return true;
----------------
`!isa<FieldDecl, CXXMethodDecl, VarDecl>(ND)`


================
Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:2906-2908
+      if (ND->getName() == MemberName)
+        return false;
+      return true;
----------------
`return ND->getName() != MemberName;`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90767