jameysharp requested cfallin for a review on PR #4939.
jameysharp opened PR #4939 from no-quadratic
to main
:
This fixes #4938/#4923, with a different take on #4924.
I think this maybe introduces worst-case quadratic behavior in
declare_block_predecessor
instead, though I think it would happen much less often. I'd appreciate any performance insights during review.I want to work some more on the comments before merging this, because it was hard to get right and involves subtle invariants. But it does pass the test suite now and I think it's correct.
jameysharp requested fitzgen for a review on PR #4939.
jameysharp updated PR #4939 from no-quadratic
to main
.
afonso360 submitted PR review.
bjorn3 created PR review comment:
Should this set in_predecessor_cycle to false?
bjorn3 submitted PR review.
afonso360 submitted PR review.
afonso360 submitted PR review.
jameysharp submitted PR review.
jameysharp created PR review comment:
No, it must already be false: there couldn't have been a cycle before because there were at least two predecessors. That's true whether
pred
is being added or removed.But it's an excellent question so I'm adding a
debug_assert
there to check this.
bjorn3 submitted PR review.
bjorn3 created PR review comment:
What if you went from
block1: jump block2 block2: jump block1
which has a predecessor cycle to
block0: jump block1 block1: jump block2 block2: jump block1
which doesn't?
jameysharp submitted PR review.
jameysharp created PR review comment:
In that example,
block1
had one predecessor (block2
), and now we're adding a second (pred
isblock0
). Thismatch
is on the number of predecessors excludingpred
, so we'd be in the one-predecessor case wherepred
gets re-assigned to point toblock2
, not this early-return case.
bjorn3 submitted PR review.
bjorn3 created PR review comment:
I see.
jameysharp updated PR #4939 from no-quadratic
to main
.
jameysharp has marked PR #4939 as ready for review.
cfallin submitted PR review.
cfallin submitted PR review.
cfallin created PR review comment:
Can we make
add
a small helper enum type, likeenum AddRemove { Add, Remove }
? That would make the meaning of the bool much clearer.
jameysharp updated PR #4939 from no-quadratic
to main
.
cfallin submitted PR review.
cfallin submitted PR review.
cfallin created PR review comment:
a tiny thing but if you add
#[derive(PartialEq, Eq)]
(or maybe justPartialEq
? I'm never clear on what needs what) to the enum, then this can beaction == Action::Add
, which I personally like a bit more for zero-arg enums (C-style enums). Totally up to you though.
jameysharp submitted PR review.
jameysharp created PR review comment:
Oh, yeah, that is definitely nicer. Thanks for the suggestion!
I think
PartialEq
is technically enough but since the derived implementation is actually total, I think it's confusing to leave offEq
, so I'm going with both.
jameysharp updated PR #4939 from no-quadratic
to main
.
jameysharp has enabled auto merge for PR #4939.
jameysharp merged PR #4939.
Last updated: Dec 23 2024 at 12:05 UTC