Fix #641#643
Conversation
|
I don't think this one should be conflict with the clippy PR, as I cherry-picked it from a branch based off of that. |
| let merge_reprs = Mark::MergeBehavior(MergeBehavior::MutuallyExclusive).repr() | ||
| | Mark::MergeBehavior(MergeBehavior::Ours).repr() | ||
| | Mark::MergeBehavior(MergeBehavior::Theirs).repr(); | ||
| // Zap just the MergeBehavior bits. |
There was a problem hiding this comment.
It would probably be better to move the merge behavior out of the mark bits,
A better design would be to have two u8 fields to replace the current u16.
Mark's which will get merged into the merge target would in one of these, and marks which
would not could go in the other.
(So, merge behaviors would go into the ephemeral pile, then we wouldn't have to strip them out)
We'd need to mess with the internal representation a little, but it allow us to remove a lot of bit fiddling.
Should at least make things a bit easier to reason about?
There was a problem hiding this comment.
Woke up with a small question of whether stripping merge behavior is always correct.
It's certainly incorrect to always set it via an assignment operator (overwriting any merge bits or marks in the merge target).
It's also incorrect to always bitwise or it, this possibly sets multiple merge behaviors for the merge target.
In this patch we never propagate merge bits. However there could be an argument for propagating them if the target has no merge behavior set.
This patch is still an improvement in that at least we're not knowingly doing incorrect things. even if we really haven't settled on all aspects of what it should be doing.
There was a problem hiding this comment.
I'm not quite sure what perfection looks like here, but this is at least an improvement, so I think we should merge now, and then consider perfecting in a subsequent PR?
There was a problem hiding this comment.
Yeah I agree, also there isn't really a place where this code path gets used by anything currently.
So this is just one step closer towards that (perfecting might be a bit strong though)
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
2 participants
This should everything in #641 which is in the grmtools impl, the 3rd issue isn't present because I don't think there are any marks in grmtools that shouldn't carry over the only marks are (required, and used), it doesn't have one like prohibited which shouldn't carry over to the merge destination.
It isn't toally clear that
usedshould carry over, but at the very least, the behavior is now consistent across merge behaviors, if we merge a value we now also merge it's mark. While keeping the merge target's marks intact, and it's merge behavior intact.This also gets rid of the
#[allow(clippy::unnecessary_unwrap)]I was looking at.