[PATCH] D80961: Ignore template instantiations if not in AsIs mode

[Manuel Klimek via Phabricator via cfe-commits]

klimek added a comment.

In D80961#2090216 <https://reviews.llvm.org/D80961#2090216>, @steveire wrote:

> In D80961#2079419 <https://reviews.llvm.org/D80961#2079419>, @aaron.ballman wrote:
>
> > In D80961#2079044 <https://reviews.llvm.org/D80961#2079044>, @klimek wrote:
> >
> > > In D80961#2076242 <https://reviews.llvm.org/D80961#2076242>, @aaron.ballman wrote:
> > >
> > > > In D80961#2073049 <https://reviews.llvm.org/D80961#2073049>, @klimek wrote:
> > > >
> > > > > Without jumping into the discussion whether it should be the default, I think we should be able to control template instantiation visitation separately from other implicit nodes.
> > > > >  Having to put AsIs on a matcher every time you need to match template instantiations is a rather big change (suddenly you have to change all the matchers you've written so far).
> > > >
> > > >
> > > > I think that's the intended meaning of `AsIs` though. Template instantiations are not source code the user wrote, they're source code the compiler stamped out from code the user wrote. I hope `IgnoreUnlessSpelledInSource` isn't a misnomer.
> > > >
> > > > > I love the idea of being able to control visitation of template instantiation.
> > > > >  I am somewhat torn on whether it should be the default, and would like to see more data.
> > > > >  I feel more strongly about needing AsIs when I want to match template instantiations.
> > > >
> > > > FWIW, my experience in clang-tidy has been that template instantiations are ignored far more often than they're desired. In fact, instantiations tend to be a source of bugs for us because they're easy to forget about when writing matchers without keeping templates in mind. The times when template instantiations become important to *not* ignore within the checks is when the check is specific to template behavior, but that's a minority of the public checks thus far.
> > >
> > >
> > > So I assume the idea is that this will work & be what we'll want people to write?
> > >  traverse(AsIs, decl(traverse(IgnoreImplicit, hasFoo)))
> >
> >
> > I believe so, yes. It's explicit about which traversal mode you want any given set of matchers to match with, so it is more flexible at the expense of being more verbose. Alternatively, you could be in `AsIs` mode for all of it and explicitly ignore the implicit nodes you wish to ignore (which may be even more verbose with the same results, depending on the matchers involved).
>


Support for clang-query is coming up, I assume? If that works & will work in clang-query that addresses my concern.

> As far as I can tell, the people who had "standing objections" to fixing this bug have been convinced that it makes sense. At least that is implied.
> 
> The problem is they haven't explicitly reversed so progress is still blocked.
> 
> Please respond to either say you're no longer blocking this or say why you continue to block it.
> 
> There is no reason not to fix this.

Stephen, I want to call this out, as I think the way you write here is not OK:

1. the scare quotes around "standing objections" reads like you're not respecting the opinions of others here; it's fine to be frustrated, it's fine to call out why you're frustrated (for example, reviews taking long time, or it being hard to find consensus), but it's not OK to use sarcasm to implicitly attack folks - the only result is that people are hurt
2. if you want somebody to chime in, please say so; implicitly alluding to people you want to respond reads as an implicit accusation of folks trying to block you; nobody wants to block you; we're professionals trying to create a great product, and we often have different opinions what that means; that's part of the process, and in the end makes for a better product
3. "There is no reason not to fix this" - asserting your believes is not an argument, and comes across as arrogant - the moment other people have objections there are reasons to object; whether those reasons in the end are more important than the reasons you cited for doing it is often a value call, and different people have different values

If you need to vent, feel free to write me (and only me) angry emails (if you preface with Vent: I also know the context, which helps :), or I'm also happy to add you to my work chat so you can vent at me in chat. I understand the need to vent - but it must not be done at the emotional expense of others on the project. In the end, behaving like this in code reviews will reduce the chance that people are willing to engage with you and spend time on reviewing your code, leading to even longer review times. This is a vicious cycle. It's in your hands to break it, but it will take time and patience.
As I've said before, I love what you're doing for the project, but ultimately, if you cannot engage constructively, we are an OSS project exactly so people who don't want to be part of the community can fork and do their own thing.
If you want to be part of the community (and I do sincerely hope that you do!), it's important to play by its rules; in the long run, it will make it a significantly better experience (and better product!) for everyone.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80961