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

[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.

LGTM!



================
Comment at: clang/lib/ASTMatchers/Dynamic/Marshallers.h:948
+          return {};
+        auto VM = Arg.Value.getMatcher();
+        if (VM.hasTypedMatcher(NK)) {
----------------
steveire wrote:
> 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 :).
> Nope, this isn't a good example of that. It's actually the opposite. auto does no harm here.

Here's a simplified example showing what I meant: https://godbolt.org/z/svbx4f. Note how the use with just `auto` executes the copy constructor while the other two forms do not.

That said, I can see your point about `VariantMatcher VM = Arg.Value.getMatcher();` having the same issues when reading the code, so you're right that this isn't a particularly good example of why not to use `auto`.

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

That's fine, I probably shouldn't have even tried to begin with. :-)


================
Comment at: clang/lib/ASTMatchers/Dynamic/Marshallers.h:988
+      *LeastDerivedKind = CladeNodeKind;
+    return true;
   }
----------------
steveire wrote:
> 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.
Ahh, thank you for the explanation, that makes more sense to me now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94879