User Details
- User Since
- Mar 28 2022, 9:12 AM (87 w, 5 d)
Aug 14 2023
Aug 11 2023
Thank you for iterating! LGTM
May 22 2023
LGTM
May 12 2023
May 11 2023
@zuban32 Not sure if you were waiting on me (if so, sorry!) - I put some comments inline that would help with the concerns we talked about previously. I'd love to get this landed once the comments are resolved :)
I'm +1 on this change for sure :) Any chance you could also do dyn_cast_or_null -> dyn_cast_if_present in that AST matcher?
Apr 26 2023
Apr 18 2023
Apr 12 2023
Apr 11 2023
The test looks fine to me, but I'm now realizing some tricky behavior. If you std::move a unique pointer into a dyn_cast, and casting fails, you now have no pointer anymore (it's been moved already). In the unit test, if BP couldn't be cast to foo I believe you'd see this behavior. I vaguely recall hitting some of this in the past, does it make sense to have UniquePtrCast take unique_ptr<> & rather than unique_ptr<> && and use .release()/.reset()?
Are there unit tests you could add to this to ensure we don't regress? Clearly we don't have good enough coverage on this.
Apr 4 2023
Mar 9 2023
Mar 6 2023
Nice, thank you.
Mar 3 2023
I'm happy to help fix up that one remaining issue (@sunho I might ask you for help and will definitely ask you for a review), would love to get this in :)
Mar 2 2023
Feb 28 2023
Nov 18 2022
Nov 15 2022
Nov 13 2022
Nov 12 2022
Nov 11 2022
Oct 14 2022
No consensus, closing this.
No consensus, closing this.
Oct 13 2022
LGTM with river's comment applied about the code comment. Thanks nick!
LGTM - I might clarify get to be getOldValue or something to make it easier on the reader at the callsite, but I don't have strong feelings about it.
Oct 12 2022
Thanks Nick :)
Oct 3 2022
LGTM, thanks River :)
Sep 28 2022
Thanks river :)
Sep 20 2022
Thanks River :)
Sep 6 2022
Sep 3 2022
Sep 2 2022
Sep 1 2022
Discourse thread here for folks that want to discuss more long-form: https://discourse.llvm.org/t/psa-swapping-out-or-null-with-if-present/65018
It's also worth noting that the _or_null variants are soft-deprecated (see comments on _or_null) and this is in preparation for actually marking them deprecated. IMO, we want one way to do the "if this value exists, do the dyn_cast/cast" and the if_present naming can apply equally well to pointers and optionals.
Aug 31 2022
Aug 30 2022
Aug 21 2022
Thanks for fixing the or_null to if_present too (where it makes sense). LGTM as long as everything passes CI :)
Aug 19 2022
Aug 17 2022
+1 to the concept, many thanks for this! I think it makes more sense to implement the member versions with the llvm-style versions, it should simplify the member style code because then you won't have to manually implement the classof/isa/cast innards it'd just be cast<T>(*this) or something.
Aug 10 2022
Aug 9 2022
Aug 8 2022
This is great, thank you for doing this! I'm not a competent reviewer for the actual clang-tidy code changes but the +1 for the idea :)
Jul 15 2022
Jul 13 2022
Jul 12 2022
Jul 10 2022
I had also considered a version of this patch that iterated the block with a set of refcounts for each value - that also works but this version felt a little closer to the way that the rest of the analysis is performed. I'm totally fine either way, happy to update the patch to use the other method.
Jun 28 2022
Thanks River :)
Jun 8 2022
Jun 6 2022
Jun 3 2022
Someone else already did this! great.