Skip to content

Fix #641 #643

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ltratt merged 1 commit into from
Jul 12, 2026
+47 −16
Merged

Fix #641 #643

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
63 changes: 47 additions & 16 deletions cfgrammar/src/lib/markmap.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -420,28 +420,42 @@ impl<K: Ord + Clone, V> MarkMap<K, V> {
let ours_mark = Mark::MergeBehavior(MergeBehavior::Ours).repr();
let exclusive_mark =
Mark::MergeBehavior(MergeBehavior::MutuallyExclusive).repr();
// Their marks with their merge behavior stripped.
let mut just_their_marks = their_mark;
let merge_reprs = Mark::MergeBehavior(MergeBehavior::MutuallyExclusive).repr()
| Mark::MergeBehavior(MergeBehavior::Ours).repr()
| Mark::MergeBehavior(MergeBehavior::Theirs).repr();
// Zap just the MergeBehavior bits.

@ratmice ratmice Jul 12, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@ratmice ratmice Jul 12, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@ltratt ltratt Jul 12, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@ratmice ratmice Jul 12, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

just_their_marks ^= just_their_marks & merge_reprs;
let mut merge_behavior = (*my_mark & exclusive_mark)
| (*my_mark & ours_mark)
| (*my_mark & theirs_mark);
if merge_behavior == 0 {
merge_behavior = Mark::MergeBehavior(self.default_merge_behavior).repr();
}
if merge_behavior == exclusive_mark {
// If only clippy could convince me and the borrow checker this is actually unnecessary.
#[allow(clippy::unnecessary_unwrap)]
if my_val.is_some() && their_val.is_some() {
return Err(MergeError::Exclusivity(
their_key,
Box::new(their_val.unwrap().into()),
));
} else if my_val.is_none() {
match their_val {
Some(their_val) if merge_behavior == exclusive_mark => {
if my_val.is_some() {
return Err(MergeError::Exclusivity(
their_key,
Box::new(their_val.into()),
));
} else {
*my_mark |= just_their_marks;
*my_val = Some(their_val.into());
}
}

their_val if merge_behavior == theirs_mark => {
*my_mark |= just_their_marks;
*my_val = their_val.map(|u| u.into());
}

their_val if my_val.is_none() && merge_behavior == ours_mark => {
*my_mark |= just_their_marks;
*my_val = their_val.map(|u| u.into());
}
} else if merge_behavior == theirs_mark && their_val.is_some()
|| merge_behavior == ours_mark && my_val.is_none()
{
*my_mark = their_mark;
*my_val = their_val.map(|u| u.into());
_ => {}
}
}
Err(pos) => {
Expand Down Expand Up @@ -795,34 +809,43 @@ mod test {
let mut theirs: MarkMap<&str, &str> = MarkMap::new();
ours.insert("a", "ours");
theirs.set_merge_behavior(&"a", MergeBehavior::MutuallyExclusive);
theirs.mark_required(&"b");
assert!(ours.merge_from(theirs).is_ok());
assert_eq!(ours.get(&"a"), Some(&"ours"));
assert!(ours.is_required(&"b"));
}
{
// Default behavior should match MutuallyExclusive
let mut ours: MarkMap<&str, &str> = MarkMap::new();
let theirs: MarkMap<&str, &str> = MarkMap::new();
let mut theirs: MarkMap<&str, &str> = MarkMap::new();
theirs.mark_required(&"b");
ours.insert("a", "ours");
assert!(ours.merge_from(theirs).is_ok());
assert_eq!(ours.get(&"a"), Some(&"ours"));
assert!(ours.is_required(&"b"));
}

{
let mut ours: MarkMap<&str, &str> = MarkMap::new();
let theirs: MarkMap<&str, &str> = MarkMap::new();
let mut theirs: MarkMap<&str, &str> = MarkMap::new();
theirs.mark_required(&"b");
ours.insert("a", "ours");
ours.set_merge_behavior(&"a", MergeBehavior::MutuallyExclusive);
assert!(ours.merge_from(theirs).is_ok());
assert_eq!(ours.get(&"a"), Some(&"ours"));
assert!(ours.is_required(&"b"));
}
{
let mut ours = MarkMap::new();
let mut theirs = MarkMap::new();
theirs.mark_required(&"b");
ours.insert("a", "ours");
ours.set_merge_behavior(&"a", MergeBehavior::Ours);

theirs.insert("a", "theirs");
assert!(ours.merge_from(theirs).is_ok());
assert_eq!(ours.get(&"a"), Some(&"ours"));
assert!(ours.is_required(&"b"));
}
}

Expand All @@ -831,28 +854,34 @@ mod test {
{
let mut ours: MarkMap<&str, &str> = MarkMap::new();
let mut theirs: MarkMap<&str, &str> = MarkMap::new();
theirs.mark_required(&"b");
ours.set_merge_behavior(&"a", MergeBehavior::MutuallyExclusive);
theirs.insert("a", "theirs");
assert!(ours.merge_from(theirs).is_ok());
assert_eq!(ours.get(&"a"), Some(&"theirs"));
assert!(ours.is_required(&"b"));
}

{
// Should match default behavior.
let mut ours: MarkMap<&str, &str> = MarkMap::new();
let mut theirs: MarkMap<&str, &str> = MarkMap::new();
theirs.mark_required(&"b");
theirs.insert("a", "theirs");
assert!(ours.merge_from(theirs).is_ok());
assert_eq!(ours.get(&"a"), Some(&"theirs"));
assert!(ours.is_required(&"b"));
}
{
let mut ours = MarkMap::new();
let mut theirs = MarkMap::new();
theirs.mark_required(&"b");
ours.insert("a", "ours");
ours.set_merge_behavior(&"a", MergeBehavior::Theirs);
theirs.insert("a", "theirs");
assert!(ours.merge_from(theirs).is_ok());
assert_eq!(ours.get(&"a"), Some(&"theirs"));
assert!(ours.is_required(&"b"));
}
}

Expand All @@ -861,13 +890,15 @@ mod test {
{
let mut ours = MarkMap::new();
let mut theirs = MarkMap::new();
theirs.mark_required(&"b");
ours.insert("a", "ours");
ours.set_merge_behavior(&"a", MergeBehavior::MutuallyExclusive);
theirs.insert("b", "theirs");
theirs.set_merge_behavior(&"b", MergeBehavior::MutuallyExclusive);
assert!(ours.merge_from(theirs).is_ok());
assert_eq!(ours.get(&"a"), Some(&"ours"));
assert_eq!(ours.get(&"b"), Some(&"theirs"));
assert!(ours.is_required(&"b"));
}
}

Expand Down