Thank you for iterating! LGTM

LGTM

In D147991#4339596, @zuban32 wrote:

<snip>
Absolutely not, I would surely ping you it was about that :) I'm sorry, this got retracted by other things. I was planning to run it with the sanitizer to see if something shows up.

In D150123#4337179, @tpopp wrote:

Assuming the rebases go smoothly, I will do this as a follow up in MLIR instead, but I will update the code at https://github.com/llvm/llvm-project/compare/main...tpopp:llvm-project:tidy-cast-check to also do this transformation, so others benefit when updating their own codebase.

@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?

In D147991#4276650, @zuban32 wrote:

@bzcheeseman I've updated the test and it passes, i.e. I wasn't able to reproduce the behavior we discussed. It seems modern c++ compilers are able to elide the calls to move constructor in such a code, I tried with gcc 9.4 and Apple aarch64 clang-14 with no success.

In D147991#4260252, @zuban32 wrote:

Oh, you're right, haven't tried that yet but I'm pretty sure that's what's going to happen. I think we could just make dyn_cast take lvalue reference to unique_ptr instead of an rvalue one, then check it with isa<>, and then either move the pointer further or just return nullptr leaving the original pointer intact. I'll update the patch

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.

In D144276#4244920, @lhames wrote:

Quick update: I'm almost done with this -- I just need to fix a couple of outstanding bugs in ELFNixPlatform and then I think this should be good to land.

Nice, thank you.

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 :)

In D137855#3923083, @rriddle wrote:

Is there a test we could write for this?

No consensus, closing this.

No consensus, closing this.

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.

Thanks Nick :)

LGTM, thanks River :)

Thanks river :)

Thanks River :)

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.

In D131500#3757581, @whisperity wrote:

Hey!

Sorry, I'm not sure how I missed it. Been on vacations and such... either way, I think documenting this is a good practice.

Do you have commit access or shall I commit on your behalf? If so, what do you want to have as the author name and e-mail for the commit?

Thanks for fixing the or_null to if_present too (where it makes sense). LGTM as long as everything passes CI :)

+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.

In D131319#3709671, @whisperity wrote:

In D131319#3708667, @bzcheeseman wrote:

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 :)

The problem with the approval here is that a single approval will turn the patch into a fully approved state, which breaks the dashboards for people added to the patch (i.e., other reviewers will think the patch is already approved, and thus perhaps will not consider putting effort into reviewing it!).

However, I think you should try the Award Token option from the menu on the right! Somewhere the awarded tokens should show up on the patch, tallying up support!

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 :)

In D129447#3656775, @rriddle wrote:

clang-format isn't happy, please look into that.

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.

Thanks River :)

In D123901#3561964, @reames wrote:

Is there any chance that this change removed the assert on cast<X> that the thing being pointed to isa<X>? I'm looking at a local case where I have cast<FixedVectorType(T) succeeding on something which clearly *isn't* a fixed vector, and isa<FixedVectorType>(T) returning false on the same pointer. Its possible that I'm looking at some weird bit of undefined behavior, but since I can't find where the corresponding assert is in the new version of the code, I wanted to ask.

Someone else already did this! great.