Awesome, thanks!

thanks!

Nice! Thank you.

In D144546#4167161, @steakhal wrote:

By looking at the title, I get the impression that this fixes an assertion violation.
I also observed that this commit is part of main but not part of release/16.x, hence the clang-16 would be released without this fix.

I want to raise awareness of backporting crash fixes to llvm releases. IMO that's a good practice.
So my question is, should we backport this patch to the release branch?

If so, could you please check if there are more commits like this for the dataflow library @ymandel?

I used git log release/16.x..main --oneline clang/lib/{Analysis,AST,ASTMatchers,StaticAnalyzer} clang/include/clang/{Analysis,AST,ASTMatchers,StaticAnalyzer} | grep -i 'crash\|fix\|assert' to check for relevant commits.

In D143128#4167626, @jkorous wrote:

This is an interesting topic. In the abstract I see the question as: "Should the Fix-Its prioritize how the code will fit the desired end state (presumably modern idiomatic C++) or carefully respect the state of the code as is now?"

The only thing I feel pretty strongly about is that no matter what philosophy we decide to use here we should apply it consistently to all our Fix-Its (which might or might not already be the case).

And FWIW I can also imagine at some point in the future we might either have two dialects of the Fix-Its or that a separate modernizer tool (completely independent of Safe Buffers) could suggest transformations like:
"Would you like to change &DRE.data()[any] to (DRE.data() + any)?"

Very sorry for the delayed response! Also, feel free to move this discussion to some other forum. Responses inline:

Thanks!

Thanks for this fix! Would you mind expanding the description a bit? I had trouble understanding the details of the problem that is being addressed. Thanks!

Do you have permissions to commit or do you need one of us to do that for you?

Looking over this patch, I'm a little concerned that you seem to have re-invented the infrastructure of RewriteRules from Transformer. Not the details of the patch itself, but the infrastructure its built on -- the FixableGadget and FixitLists all seem very similar. While you're welcome to do as you please, I imagine the project would be best off if there was a common infrastructure. Have you taken a look at clang::transformer? If so, is there a reason that it doesn't meet your needs? https://github.com/llvm/llvm-project/blob/main/clang/include/clang/Tooling/Transformer/RewriteRule.h is the starting point, if you're not familiar with it. The doc page is here: https://intel.github.io/llvm-docs/clang/ClangTransformerTutorial.html.

good catch!

address comments

add fixme and fix compilation error

Just realized this is actually more complicated to change, assuming I also want to update all the derived classes. For now, I'll limit the scope of the change to these files, but I'll follow up with an overload-based solution to admit either kind of derived class until we can port them all over to then new signature.

In D142710#4097032, @xazax.hun wrote:

In D142710#4096325, @ymandel wrote:

In D142710#4094934, @xazax.hun wrote:

This change looks good to me. I wonder, however, whether the behavior should be parameterized in the future. E.g., whether the user of the analysis should be able to make a decision whether the analysis should be pessimistic or optimistic about unmodeled values.

Interesting idea. I think this goes along with other places where we are unsound. Here, we err on the side of soundness. but, in general, we should have a configuration mechanism for this. FWIW, the only reason we have uninitialized values at this point is recursive types. We also limit the depth of structs, but that should be removed given my recent patch to only model relevant fields. I have an idea for lazy initialization of values that I think could solve the recursion issue. Together, we could remove this concept of unmodeled values altogether from the framework.

Oh, sounds great! I do think lazy initialization will be really valuable to reduce the number of unmodeled values, but not entirely sure if we can completely eliminate them. In case we end up creating new locations (different from the earlier ones) in every iteration of the loop it might be harder to reach a fixed point.

In D142710#4094934, @xazax.hun wrote:

This change looks good to me. I wonder, however, whether the behavior should be parameterized in the future. E.g., whether the user of the analysis should be able to make a decision whether the analysis should be pessimistic or optimistic about unmodeled values.

In D140897#4094945, @xazax.hun wrote:

I hope we will be able to get rid of this SkipPast thing at some point by looking at the value categories of the AST instead.

Rebase onto HEAD

Gentle ping.

use isa_and_nonnull

In D140434#4071823, @carlosgalvezp wrote:

In D140434#4053053, @suertreus wrote:

Thanks for reviewing.

I don't have commit access; can someone who does please do the thing?

@suertreus I can help you land the patch, what user name and email should I use for attribution?

Sure, please ping me if you don't see a commit by EOD

address comments

Thanks!

Can you add a test? probably valuable even if its nearly identical to the test for getRangeForEdit.

use std::optional

Initialize AnalysisOptions as a struct, avoiding a use-after-move bug from incorrect use of the fluent (rvalue-ref) API.

rebasing.

Committed: https://reviews.llvm.org/rG01ccf7b3cee58dbe02fd97696cae1781746b6137

fix memory error that cause buildbot failures

fix use-after-free bug in test. Possible cause of buildbot failures.

Abandoning in favor of https://reviews.llvm.org/D140694.

I'll revert now and take a look in the morning...

fix broken test (from rebase)

fix typo

In D140694#4026244, @xazax.hun wrote:

This is a big improvement, I skimmed through it and looks good to me. There might be some sneaky ways to change values of fields without mentioning them, e.g., casting a pointer to a struct to an array of chars. But I think reasoning about those scenarios is probably not something we want to model anytime soon.

Thanks!

address comments

rebase onto HEAD

add clarifying comment regarding interpration of DecompositionDecl.

Gabor -- do you want to review before I land this patch (Dmitri already accepted it)? thx

Thanks!