[PATCH] D80499: Remove obsolete ignore*() matcher uses

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added a comment.

In D80499#2352339 <https://reviews.llvm.org/D80499#2352339>, @steveire wrote:

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80499