komalverma04 wrote: > 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: [#75754 > (comment)](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).
Thank you so much for making me aware of these critical aspects. I will do the needed changes. 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
