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.

In D126446#3553562, @jfurtek wrote:

The current implementation uses hexadecimal, which requires 16 bits (2 bytes) to encode 8 bits of binary data. base64 would be an improvement over the current implementation that uses hexadecimal, requiring 8 bits to encode 6 bits of binary data. (The other constraint here is that encoding needs to be handled by the parser, which in the original example, means it can't have a quotation mark.)

In D126446#3548652, @jfurtek wrote:

Have you considered using base64 instead of hex for binary data?

In D123901#3530352, @sylvestre.ledru wrote:

@bzcheeseman FYI, this is breaking the build with gcc 6 (for Debian stretch)
if you are aware of a simple fix, it would be appreciated
See https://github.com/llvm/llvm-project/issues/55626

(I am not sure I want to backport gcc 7 or 8 to Debian stretch ;)

thanks

Not a qualified reviewer for anything other than the llvm::isa usage and that line looks good to me!

In D125709#3519033, @bzcheeseman wrote:

In D125709#3518096, @steakhal wrote:

I had to fix the doCast to return To instead of Optional<To> to make it work.

That's fine (or it should be!), you could dereference the optional if you wanted to

Currently, we expect that casts result in regular SVal objects, instead of pointer-like objects , thus this code to compile:
NonLoc N = llvm::cast<NonLoc>(V), where V is of type SVal. I believe that is why I decided to make that change.

In D125543#3518277, @ftynse wrote:

This will not happen for a random class that does not have CastInfo specialized though. For Operation *, the compiler picks the CastInfo<T, ::mlir::Operation *> specialization over the more generic CastInfo<T, T>, I don't think there is an easy way around this.

In D125709#3518096, @steakhal wrote:

I had to fix the doCast to return To instead of Optional<To> to make it work.

In D125543#3517778, @rriddle wrote:

I'm fine with landing this to unbreak things, but we really shouldn't need to define this IMO (the infra should automatically handle it).

@ftynse https://reviews.llvm.org/D125590 should fix this - can you please confirm and let me know if not?

Thank you for working on this!