Page MenuHomePhabricator

Remove obsolete ignore*() matcher uses
AcceptedPublic

Authored by steveire on May 24 2020, 4:23 PM.

Details

Reviewers
aaron.ballman

Diff Detail

Event Timeline

steveire created this revision.May 24 2020, 4:23 PM
aaron.ballman accepted this revision.May 25 2020, 12:45 PM

This is *great*, thank you for working on the cleanup! LGTM

This revision is now accepted and ready to land.May 25 2020, 12:45 PM

@alexfh This change is based on the behavior of AST Matchers being changed to ignore invisible/implicit AST nodes by default. As the default was not changed in the end, this patch would need to be updated to add traverse(TK_IgnoreUnlessSpelledInSource) wrapping around the matchers.

Before I update the patch to add that, do you have any feedback?

@alexfh This change is based on the behavior of AST Matchers being changed to ignore invisible/implicit AST nodes by default. As the default was not changed in the end, this patch would need to be updated to add traverse(TK_IgnoreUnlessSpelledInSource) wrapping around the matchers.

Before I update the patch to add that, do you have any feedback?

If the default is not being changed, clear improvement will be noticeable only where multiple ignore.* matchers are being replaced with a traverse(TK_IgnoreUnlessSpelledInSource). However, in many cases the behavior of the checks will change (and I'm not sure we have good enough test coverage especially where implicit conversions and similar are present). I believe, we're going to hit a number of corner cases and uncover a ton of bugs with this change. (Not that it would be different, if the default was changed to ignore implicit stuff ;)

You should be ready for back and forth with this change, if users hit widespread issues not caught by tests. Maybe splitting it into separate pieces and involving check authors where practical may be reasonable.

! In D80499#2353187, @alexfh wrote:
You should be ready for back and forth with this change, if users hit widespread issues not caught by tests. Maybe splitting it into separate pieces and involving check authors where practical may be reasonable.

Ok, I'll do that when https://reviews.llvm.org/D80961 and https://reviews.llvm.org/D82278 are merged.

! In D80499#2353187, @alexfh wrote:
You should be ready for back and forth with this change, if users hit widespread issues not caught by tests. Maybe splitting it into separate pieces and involving check authors where practical may be reasonable.

Ok, I'll do that when https://reviews.llvm.org/D80961 and https://reviews.llvm.org/D82278 are merged.

@alexfh This is done in D91302 and D91303. Could you review?