https://github.com/5chmidti requested changes to this pull request.
Thanks for looking into this. Because this is touching a lot of checks, there was bound to be some conversation about which matchers need the `ignoringParenImpCasts` and which don't. I think we should check that now instead of later, so don't be alarmed about the number of comments. I'm just trying to make sure we only add `ignoringParenImpCasts` where it is needed. Maybe other reviewers can chime in about this and confirm my comments before you act on them, in case there are any that are incorrect or if we defer this for a later cleanup pr. That way, you don't do work that is potentially undone again. I added these comments by only looking at these matchers. While I hope all are correct, there may be some that are not, so let me know. Those matchers(/checks) do not care about the implicit nodes or only care about a type in a certain way, so they can be removed. However, there are some, where I think that removing the `ignoringParenImpCasts` may actually fix issues related to extra parentheses added by the user, if there are fix-it's involved (e.g., `UseStartsEndsWithChecl.cpp`). Please add a release note to: https://github.com/llvm/llvm-project/blob/main/clang/docs/ReleaseNotes.rst#ast-matchers about the change to the AST matcher. The changes to the matchers (in e.g. clang-tidy checks) don't need a release note, because they should be essentially non-functional changes (IMO). w.r.t. formatting: The general stance on formatting in LLVM is to only format in the vicinity of a committed change (think 'only the line +- a few lines if it makes sense'). Please clean up unrelated formatting changes from this pr. Check your editor's settings regarding formatting. There should probably be a setting available to only format changed lines (which you should also be able to only enable for LLVM instead of all of your projects). Regarding the failing CI: There are more consumers of AST matchers than just clang-tidy checks. If you search through the failure log, you'll see which tests are failing (or checkout this: https://github.com/llvm/llvm-project/issues/75754#issuecomment-1913568669), and you can backtrack from there. You could also search for `hasArgument` outside the `clang-tidy` folder. The relevant tests to run should be: `ninja check-clang check-clang-tools` to get everything. It is also what the CI runs (minus not relevant tests to this pr). https://github.com/llvm/llvm-project/pull/89553 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
