[PATCH] D72530: Set traversal explicitly where needed in clang-tidy

Sun May 17 08:30:56 PDT 2020

aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

In D72530#2040521 <https://reviews.llvm.org/D72530#2040521>, @steveire wrote:

> In D72530#2038561 <https://reviews.llvm.org/D72530#2038561>, @aaron.ballman wrote:
>
> > The changes here look reasonable, but can some of this code be simplified to use the default behavior and less complex matchers that don't have to carefully ignore implicit nodes? This would demonstrate that the feature really is an improvement over the status quo and would also exercise more test code with the new defaults demonstrating equivalence. I'm not envisioning anything onerous, more just wondering if you think there is some low-hanging fruit that could be picked rather than converting everything to AsIs.
>
>
> After changing the default there will be opportunities for that as a follow-up, yes. But this patch is ordered before changing the default. The change you describe is a follow-up. What I am doing with that ordering is called "non-atomic refactoring".
>
> Here's an example of such a future change: https://gist.github.com/steveire/32fbf457377bc6c57bd3906cc9c8fb9c

Thanks for the example, that's the kind of improvement I was hoping to see. Given that one of the claims in the RFC is that this makes writing matchers easier, I think it's reasonable to expect a patch showing that it actually does make writing those matchers more simple. I suppose I'm fine if it's a separate patch from this one, but I'd slightly prefer it was done in this patch so that we don't churn the code unnecessarily (and it makes it easier to do the comparison between the original code and the code for simplified matcher, as your changes here are adding interim complexity). Regardless, I'd like to see a patch that shows some of these simplifications as part of this patch set (if not this specific review).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72530