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