[PATCH] D94879: Implement dynamic mapAnyOf in terms of ASTNodeKinds

[Stephen Kelly via Phabricator via cfe-commits]

steveire added inline comments.


================
Comment at: clang/lib/ASTMatchers/Dynamic/Marshallers.h:948
+          return {};
+        auto VM = Arg.Value.getMatcher();
+        if (VM.hasTypedMatcher(NK)) {
----------------
aaron.ballman wrote:
> Note, this is an example of why use of `auto` is discouraged in the project when the type is not spelled out explicitly in the initialization -- this was accidentally creating a non-const copy of the `VariantMatcher`, not getting a const reference as `getMatcher()` returns. Not the worst problem in the world, but it takes a lot of review effort to find these issues when you use `auto`, which is why the guidance is what it is.
> Note, this is an example of why use of auto is discouraged 

Nope, this isn't a good example of that. It's actually the opposite. `auto` does no harm here.

If I had written 

```
    VariantMatcher VM = Arg.Value.getMatcher();
```

you would either already-know what the return type of `getMatcher()` is and see the copy, or you would be satisfied that the variable type is not `auto` (dangerously, potentially) and move on, or you would go and check the return type of `getMatcher()` if you had a suspicion.

If I had written 

```
    SomeTypedefName VM = Arg.Value.getMatcher();
```

you wouldn't see an `auto`, which again might be satisfying, but you would have to go and look at the typedef to see if it contains a `const` or a `&` (for example see `ValueParamT` in `SmallVector` which is either `const T&` or `T`, depending on `T`).

Requiring non-use of `auto` is not a way around knowing or checking the return value of methods, and can actually give you a false sense of security! 

I don't think you'll ever convince me that your way doesn't make the code worse :).


================
Comment at: clang/lib/ASTMatchers/Dynamic/Marshallers.h:950
+        if (VM.hasTypedMatcher(NK)) {
+          auto DM = VM.getTypedMatcher(NK);
+          InnerArgs.push_back(DM);
----------------
aaron.ballman wrote:
> Rather than have a has/get pair, would it make sense to have a get method that returns an `Optional` so that we don't have to check the same thing twice?
I think you're talking about the `VariantMatcher` API, so I think that's out of scope of this patch.


================
Comment at: clang/lib/ASTMatchers/Dynamic/Marshallers.h:988
+      *LeastDerivedKind = CladeNodeKind;
+    return true;
   }
----------------
aaron.ballman wrote:
> We used to traverse the list of matcher functions to see if one is convertible or not and now we're always assuming that the conversion is fine -- was that previous checking unnecessary or has it been subsumed by checking somewhere else?
Yes, the checking was not necessary. Because this matcher is basically a convenient implementation of `stmt(anyOf(ifStmt(innerMatchers), forStmt(innerMatchers)))`, it's the outer `stmt` that it is convertible to.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94879