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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
