This is an archive of the discontinued LLVM Phabricator instance.

Set traversal explicitly where needed in clang-tidy
ClosedPublic

Authored by steveire on Jan 10 2020, 11:49 AM.

Event Timeline

steveire created this revision.Jan 10 2020, 11:49 AM

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.

steveire added a comment.EditedMay 17 2020, 3:43 AM

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

aaron.ballman accepted this revision.May 17 2020, 8:03 AM

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

This revision is now accepted and ready to land.May 17 2020, 8:03 AM
This revision was automatically updated to reflect the committed changes.