immunant
wip rewrite address-taken locals
I checked most of the larger graphs in the snapshot diff and they all look good to me.
Have you tested the new AddrOfLocal handling on code with loops or other interesting control flow? The current strategy involves special handling for the first initialization of a variable, and that makes me a bit nervous. Some things I'd particularly like to see:
- A variable declared and initialized inside a loop:
loop { let x = ...; ... &x ... } - A variable initialized with a conditional:
let x = if cond { ... } else { ... }; ... &x ... - A variable that's only conditionally initialized:
let mut x; if cond { x = 1; ... &x ... }; x = 2; ... &x ...
What does this PR do? Could you update PR title and add a brief description?
Have you tested the new AddrOfLocal handling on code with loops or other interesting control flow?
No, I will add more snapshot tests for those cases and update you when they are ready.
What does this PR do? Could you update PR title and add a brief description?
Will do soon, will definitely have it by the time I mark it ready for review for you and others.
Will do soon, will definitely have it by the time I mark it ready for review for you and others.
Okay, great. I wasn't sure since @spernsteiner already approved.
Okay, great. I wasn't sure since @spernsteiner already approved.
Yeah, I won't be merging until I have approval for the PR generally. That was just for the snapshot.
Is there a way to split this PR up more? It's getting quite big. The formatting changes in pointers.rs can at least be done directly to master; I've been meaning to do that.
Is there a way to split this PR up more? It's getting quite big. The formatting changes in pointers.rs can at least be done directly to master; I've been meaning to do that.
@kkysen I'd rather not, the parts that could be split out are pretty small in comparison to the one big feature. If that were broken up, it would mean merging incomplete functionality / incorrect PDG objects, and so I think it makes sense to land all at once.
Also, after #637 landed, we have a release snapshot in addition to the existing debug one, so can you add the updates to that one here?
Also, after https://github.com/immunant/c2rust/pull/637 landed, we have a release snapshot in addition to the existing debug one, so can you add the updates to that one here?
Done
@fw-immunant @kkysen i've squashed things into one commit and will split it up from here, that seems easier than approaching it from the opposite direction