[PATCH] D95714: [clang-tidy] fix modernize-use-nullptr false positive with spaceship operator comparisons

This revision was automatically updated to reflect the committed changes. Closed by commit rG98146c1f5d0c: [clang-tidy] fix modernize-use-nullptr false positive with spaceship operator… (authored by poelmanc, committed by njames93). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

[PATCH] D95714: [clang-tidy] fix modernize-use-nullptr false positive with spaceship operator comparisons

poelmanc updated this revision to Diff 322302. poelmanc added a comment. Capitalize `IsOrHasDescendant`, add `} namespace std` per HarborMaster feedback. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D95714/new/ https://reviews.llvm.org/D95714

[PATCH] D95714: [clang-tidy] fix modernize-use-nullptr false positive with spaceship operator comparisons

poelmanc updated this revision to Diff 322253. poelmanc added a comment. Thanks for your patience, this one should work, as I used my normal `git show HEAD -U99` workflow. (The requested change was just to add a period to a comment, so as a short-cut I tried "Raw Diff" -> copy -> "Update

[PATCH] D95714: [clang-tidy] fix modernize-use-nullptr false positive with spaceship operator comparisons

njames93 added a comment. I'm unable to cleanly apply this patch, looks like something was a bit off when you created the diff. ➜ llvm-project git:(main) ✗ arc patch D95714 INFO Base commit is not in local repository; trying to fetch. Created and checked out

[PATCH] D95714: [clang-tidy] fix modernize-use-nullptr false positive with spaceship operator comparisons

poelmanc added a comment. In D95714#2548735 , @njames93 wrote: > LGTM, Same as last time for the commit? That would be great, thanks! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D95714/new/ https://reviews.llvm.org/D95714

[PATCH] D95714: [clang-tidy] fix modernize-use-nullptr false positive with spaceship operator comparisons

njames93 accepted this revision. njames93 added a comment. This revision is now accepted and ready to land. Herald added a subscriber: nullptr.cpp. LGTM, Same as last time for the commit? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D95714/new/ https://reviews.llvm.org/D95714

[PATCH] D95714: [clang-tidy] fix modernize-use-nullptr false positive with spaceship operator comparisons

poelmanc added a comment. In D95714#2540685 , @njames93 wrote: > Can I ask if you could tidy the description of this, basically remove all the > stuff about hasGrandparent etc, probably best just remove everything after > `result = (a1 nullptr a2);` in

[PATCH] D95714: [clang-tidy] fix modernize-use-nullptr false positive with spaceship operator comparisons

njames93 added a comment. In D95714#2540626 , @poelmanc wrote: > Thanks for all the great feedback I received here. To give credit where > credit's due, this updated revision to UseNullptrCheck.cpp is now actually > 100% @steveire's //suggested// code.

[PATCH] D95714: [clang-tidy] fix modernize-use-nullptr false positive with spaceship operator comparisons

poelmanc added a comment. Thanks for all the great feedback I received here. To give credit where credit's due, this updated revision to UseNullptrCheck.cpp is now actually 100% @steveire's //suggested// code. Even one of the tests cases was his. Whenever it's ready to land I'd appreciate it

[PATCH] D95714: [clang-tidy] fix modernize-use-nullptr false positive with spaceship operator comparisons

poelmanc updated this revision to Diff 320637. poelmanc added a comment. Add period to end of comment. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D95714/new/ https://reviews.llvm.org/D95714 Files: clang-tools-extra/clang-tidy/modernize/UseNullptrCheck.cpp

[PATCH] D95714: [clang-tidy] fix modernize-use-nullptr false positive with spaceship operator comparisons

aaron.ballman added a comment. In D95714#2532957 , @poelmanc wrote: > In D95714#2532565 , @njames93 wrote: > >> This does highlight an issue where the mimicked std library stubs used for >> tests don't correspond

[PATCH] D95714: [clang-tidy] fix modernize-use-nullptr false positive with spaceship operator comparisons

poelmanc updated this revision to Diff 320393. poelmanc added a comment. Thanks @steveire, that suggestion worked perfectly! I added the additional test case and shortened the mimicked `strong_ordering` code to a version from `clang/unittests.ASTMatchers/ASTMatchersTraversalTest.cpp`, but also

[PATCH] D95714: [clang-tidy] fix modernize-use-nullptr false positive with spaceship operator comparisons

steveire added a comment. In D95714#2532957 , @poelmanc wrote: > Thoughts on which option seems like the best path forward? I think you should be able to correctly match everything. Try a matcher like (can probably be cleaned up a bit): auto

[PATCH] D95714: [clang-tidy] fix modernize-use-nullptr false positive with spaceship operator comparisons

poelmanc added a comment. In D95714#2532565 , @njames93 wrote: > This does highlight an issue where the mimicked std library stubs used for > tests don't correspond exactly to what the stdlib actually looks like and can > result in subtly different ASTs

[PATCH] D95714: [clang-tidy] fix modernize-use-nullptr false positive with spaceship operator comparisons

njames93 added a comment. In D95714#2532516 , @poelmanc wrote: > @njames93 Thank you so much for the quick feedback, I made your suggested > changes and added a test that it properly converts `result = (a1 > (ptr == 0 > ? a1 : a2));` to `result = (a1 >

[PATCH] D95714: [clang-tidy] fix modernize-use-nullptr false positive with spaceship operator comparisons

poelmanc added a comment. @njames93 Thank you so much for the quick feedback, I made your suggested changes and added a test that it properly converts `result = (a1 > (ptr == 0 ? a1 : a2));` to `result = (a1 > (ptr == nullptr ? a1 : a2));` now. In these examples so far, checking the

[PATCH] D95714: [clang-tidy] fix modernize-use-nullptr false positive with spaceship operator comparisons

poelmanc updated this revision to Diff 320330. poelmanc added a comment. Thanks to the great feedback, changed `unless(hasAncestor(cxxRewrittenBinaryOperator()))` to `unless(hasParent(expr(hasParent(cxxRewrittenBinaryOperator()` and added a test to verify the improvement (and removed an