[PATCH] D71846: [ASTMatchers] Fix for https://bugs.llvm.org/show_bug.cgi?id=44364

aaron.ballman closed this revision. aaron.ballman added a comment. Thank you for the patch, I've committed on your behalf in ec3d8e61b527c6312f77a4dab095ffc34e954927 CHANGES SINCE LAST ACTION

[PATCH] D71846: [ASTMatchers] Fix for https://bugs.llvm.org/show_bug.cgi?id=44364

njames93 added a comment. Yes please, I'm still very new to the llvm contributing system so I would have no idea what to do next. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71846/new/ https://reviews.llvm.org/D71846 ___ cfe-commits

[PATCH] D71846: [ASTMatchers] Fix for https://bugs.llvm.org/show_bug.cgi?id=44364

aaron.ballman accepted this revision. aaron.ballman added a comment. Still LG -- do you need someone to land this on your behalf? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71846/new/ https://reviews.llvm.org/D71846 ___ cfe-commits

[PATCH] D71846: [ASTMatchers] Fix for https://bugs.llvm.org/show_bug.cgi?id=44364

njames93 updated this revision to Diff 235791. njames93 added a comment. small reformat CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71846/new/ https://reviews.llvm.org/D71846 Files: clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.cpp

[PATCH] D71846: [ASTMatchers] Fix for https://bugs.llvm.org/show_bug.cgi?id=44364

njames93 marked an inline comment as done. njames93 added a comment. In D71846#1800503 , @aaron.ballman wrote: > The new option LGTM but one of the tests can be updated to have a less > complex RUN line. that was just put in when i was initially

[PATCH] D71846: [ASTMatchers] Fix for https://bugs.llvm.org/show_bug.cgi?id=44364

njames93 updated this revision to Diff 235788. njames93 added a comment. removed the unnecessary explicit setting of WarnOnUnfixable value for test case CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71846/new/ https://reviews.llvm.org/D71846 Files:

[PATCH] D71846: [ASTMatchers] Fix for https://bugs.llvm.org/show_bug.cgi?id=44364

aaron.ballman accepted this revision. aaron.ballman added a comment. The new option LGTM but one of the tests can be updated to have a less complex RUN line. Comment at: clang-tools-extra/test/clang-tidy/checkers/readability-else-after-return.cpp:2-3 +// RUN:

[PATCH] D71846: [ASTMatchers] Fix for https://bugs.llvm.org/show_bug.cgi?id=44364

njames93 updated this revision to Diff 235775. njames93 added a comment. Added option to disable warning when an automatic fix can't be applied due to scope restrictions of variables, default option is to show all warnings CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71846/new/

[PATCH] D71846: [ASTMatchers] Fix for https://bugs.llvm.org/show_bug.cgi?id=44364

njames93 added a comment. In D71846#1800481 , @aaron.ballman wrote: > In D71846#1800480 , @njames93 wrote: > > > Definitely default to diagnosing everything, that's how my current > > implementation looks right

[PATCH] D71846: [ASTMatchers] Fix for https://bugs.llvm.org/show_bug.cgi?id=44364

aaron.ballman added a comment. In D71846#1800480 , @njames93 wrote: > In D71846#1800411 , @aaron.ballman > wrote: > > > In D71846#1800401 , @njames93 > > wrote: > > > > >

[PATCH] D71846: [ASTMatchers] Fix for https://bugs.llvm.org/show_bug.cgi?id=44364

njames93 added a comment. In D71846#1800411 , @aaron.ballman wrote: > In D71846#1800401 , @njames93 wrote: > > > In D71846#1800381 , @aaron.ballman > > wrote: > > > > > In

[PATCH] D71846: [ASTMatchers] Fix for https://bugs.llvm.org/show_bug.cgi?id=44364

aaron.ballman added a comment. In D71846#1800401 , @njames93 wrote: > In D71846#1800381 , @aaron.ballman > wrote: > > > In D71846#1800344 , @njames93 > > wrote: > > > > >

[PATCH] D71846: [ASTMatchers] Fix for https://bugs.llvm.org/show_bug.cgi?id=44364

njames93 added a comment. In D71846#1800381 , @aaron.ballman wrote: > In D71846#1800344 , @njames93 wrote: > > > I'm in two minds about issuing a warning when scope restrictions prevent a > > fix. Do you think

[PATCH] D71846: [ASTMatchers] Fix for https://bugs.llvm.org/show_bug.cgi?id=44364

aaron.ballman added a comment. In D71846#1800344 , @njames93 wrote: > I'm in two minds about issuing a warning when scope restrictions prevent a > fix. Do you think creating an option to enable or disable emitting warnings > for cases where the scope

[PATCH] D71846: [ASTMatchers] Fix for https://bugs.llvm.org/show_bug.cgi?id=44364

njames93 added a comment. I'm in two minds about issuing a warning when scope restrictions prevent a fix. Do you think creating an option to enable or disable emitting warnings for cases where the scope prevents a fix would be a good idea? CHANGES SINCE LAST ACTION

[PATCH] D71846: [ASTMatchers] Fix for https://bugs.llvm.org/show_bug.cgi?id=44364

njames93 updated this revision to Diff 235752. njames93 added a comment. hopefully adhered to all conventions :) CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71846/new/ https://reviews.llvm.org/D71846 Files: clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.cpp

[PATCH] D71846: [ASTMatchers] Fix for https://bugs.llvm.org/show_bug.cgi?id=44364

aaron.ballman added a comment. In D71846#1798115 , @njames93 wrote: > Sorry I didn't make it obvious with the test cases. The fix will never extend > the lifetime of variables either in the condition or declared in the else > block. It will only apply

[PATCH] D71846: [ASTMatchers] Fix for https://bugs.llvm.org/show_bug.cgi?id=44364

njames93 updated this revision to Diff 235661. njames93 added a comment. Fixed a few of the test cases failing CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71846/new/ https://reviews.llvm.org/D71846 Files: clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.cpp

[PATCH] D71846: [ASTMatchers] Fix for https://bugs.llvm.org/show_bug.cgi?id=44364

njames93 updated this revision to Diff 235573. njames93 added a comment. Fixed a few more edge cases with test cases CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71846/new/ https://reviews.llvm.org/D71846 Files: clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.cpp

[PATCH] D71846: [ASTMatchers] Fix for https://bugs.llvm.org/show_bug.cgi?id=44364

njames93 added a comment. Adding checks to see if there are any declarations already in the ifs contained scope is really starting to bloat the code while trying to cover every edge case. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71846/new/ https://reviews.llvm.org/D71846

[PATCH] D71846: [ASTMatchers] Fix for https://bugs.llvm.org/show_bug.cgi?id=44364

njames93 added a comment. Sorry I didn't make it obvious with the test cases. The fix will never extend the lifetime of variables either in the condition or declared in the else block. It will only apply if the if is the last statement in its scope. Though I should check back through the scope

[PATCH] D71846: [ASTMatchers] Fix for https://bugs.llvm.org/show_bug.cgi?id=44364

aaron.ballman added a comment. In D71846#1796883 , @njames93 wrote: > So I wasn't happy with the vagueness of the else after return check > implementation. Almost finished rewriting the check to properly handle the if > statements with condition

[PATCH] D71846: [ASTMatchers] Fix for https://bugs.llvm.org/show_bug.cgi?id=44364

njames93 added a comment. Should also point out that clang-format now has a better time reformatting as i expanded the replacements range to include the entire else block rather than the start and end braces CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71846/new/

[PATCH] D71846: [ASTMatchers] Fix for https://bugs.llvm.org/show_bug.cgi?id=44364

njames93 updated this revision to Diff 235384. njames93 edited the summary of this revision. njames93 added a comment. Think all the test cases are working, but they look like they could do with a clean up from someone who may know the clang-tidy-checks system better than myself CHANGES SINCE

[PATCH] D71846: [ASTMatchers] Fix for https://bugs.llvm.org/show_bug.cgi?id=44364

njames93 added a comment. So I wasn't happy with the vagueness of the else after return check implementation. Almost finished rewriting the check to properly handle the if statements with condition variables based on scope restrictions and where decls are used etc. int

[PATCH] D71846: [ASTMatchers] Fix for https://bugs.llvm.org/show_bug.cgi?id=44364

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM aside from a minor style nit. Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:4325-4326 + internal::Matcher,

[PATCH] D71846: [ASTMatchers] Fix for https://bugs.llvm.org/show_bug.cgi?id=44364

njames93 updated this revision to Diff 235301. njames93 edited the summary of this revision. njames93 added a comment. fixed a spelling mistake in unit tests CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71846/new/ https://reviews.llvm.org/D71846 Files:

[PATCH] D71846: [ASTMatchers] Fix for https://bugs.llvm.org/show_bug.cgi?id=44364

njames93 updated this revision to Diff 235300. njames93 marked an inline comment as done. njames93 added a comment. Renamed 'hasInitStorage' to 'hasInitStatement'. Added support for range for loops in 'hasInitStatement'. 'hasInitStatement' now has a match predicate on the init statement.

[PATCH] D71846: [ASTMatchers] Fix for https://bugs.llvm.org/show_bug.cgi?id=44364

njames93 updated this revision to Diff 235278. njames93 added a comment. I added test cases for the matchers and the check, wasn't able to add the CXXRangeFor to the matcher as that class uses a different dialect for the initializer statement handling. I also couldn't generate the new

[PATCH] D71846: [ASTMatchers] Fix for https://bugs.llvm.org/show_bug.cgi?id=44364

njames93 added a comment. Thanks for the pointers, I'll amend those changes and update the patch, thanks for pointing it out for me. Probably be after boxing day though Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71846/new/

[PATCH] D71846: [ASTMatchers] Fix for https://bugs.llvm.org/show_bug.cgi?id=44364

aaron.ballman requested changes to this revision. aaron.ballman added a comment. This revision now requires changes to proceed. Thank you for this! There's some missing test coverage for the changes to the clang-tidy check and for the new matcher. Also, the new matcher should be registered in

[PATCH] D71846: [ASTMatchers] Fix for https://bugs.llvm.org/show_bug.cgi?id=44364

njames93 added a comment. It does support the new range for loop, I'll add in a case for that in the AST matcher Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71846/new/ https://reviews.llvm.org/D71846

[PATCH] D71846: [ASTMatchers] Fix for https://bugs.llvm.org/show_bug.cgi?id=44364

malcolm.parsons added a comment. Does Clang support C++20's range-based for statements with initializer? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71846/new/ https://reviews.llvm.org/D71846 ___