Skip to content
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

[red-knot] Use BitSet::union for merging of declarations #15451

Merged
merged 1 commit into from
Jan 13, 2025
Merged

[red-knot] Use BitSet::union for merging of declarations #15451

Show file tree
Hide file tree
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
66 changes: 66 additions & 0 deletions crates/red_knot_python_semantic/src/semantic_index/use_def/bitset.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,19 @@ impl<const B: usize> BitSet<B> {
}
}

/// Union in-place with another [`BitSet`].
pub(super) fn union(&mut self, other: &BitSet<B>) {
Comment on lines +96 to +97
Copy link
Member

Choose a reason for hiding this comment

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

nit that is probably not even worth a followup PR: I'd have probably called this something like union_with (similar to https://docs.rs/bit-set/latest/bit_set/struct.BitSet.html#method.union_with) since union methods on other structs seem to generally return a new value rather than working in-place (https://doc.rust-lang.org/std/collections/struct.HashSet.html#method.union, https://docs.rs/bitflags/latest/bitflags/trait.Flags.html#method.union, https://docs.rs/bit-set/latest/bit_set/struct.BitSet.html#method.union)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. I just put some previously-approved code back in place that was there before, which is why I didn't wait for any other reviews. But that seems like a reasonable improvement.

let mut max_len = self.blocks().len();
let other_len = other.blocks().len();
if other_len > max_len {
max_len = other_len;
self.resize_blocks(max_len);
}
for (my_block, other_block) in self.blocks_mut().iter_mut().zip(other.blocks()) {
*my_block |= other_block;
}
}

/// Return an iterator over the values (in ascending order) in this [`BitSet`].
pub(super) fn iter(&self) -> BitSetIterator<'_, B> {
let blocks = self.blocks();
Expand Down Expand Up @@ -222,6 +235,59 @@ mod tests {
assert_bitset(&b1, &[89]);
}

#[test]
fn union() {
let mut b1 = BitSet::<1>::with(2);
let b2 = BitSet::<1>::with(4);

b1.union(&b2);
assert_bitset(&b1, &[2, 4]);
}

#[test]
fn union_mixed_1() {
let mut b1 = BitSet::<1>::with(4);
let mut b2 = BitSet::<1>::with(4);
b1.insert(89);
b2.insert(5);

b1.union(&b2);
assert_bitset(&b1, &[4, 5, 89]);
}

#[test]
fn union_mixed_2() {
let mut b1 = BitSet::<1>::with(4);
let mut b2 = BitSet::<1>::with(4);
b1.insert(23);
b2.insert(89);

b1.union(&b2);
assert_bitset(&b1, &[4, 23, 89]);
}

#[test]
fn union_heap() {
let mut b1 = BitSet::<1>::with(4);
let mut b2 = BitSet::<1>::with(4);
b1.insert(89);
b2.insert(90);

b1.union(&b2);
assert_bitset(&b1, &[4, 89, 90]);
}

#[test]
fn union_heap_2() {
let mut b1 = BitSet::<1>::with(89);
let mut b2 = BitSet::<1>::with(89);
b1.insert(91);
b2.insert(90);

b1.union(&b2);
assert_bitset(&b1, &[89, 90, 91]);
}

#[test]
fn multiple_blocks() {
let mut b = BitSet::<2>::with(120);
Expand Down
21 changes: 11 additions & 10 deletions crates/red_knot_python_semantic/src/semantic_index/use_def/symbol_state.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -316,6 +316,9 @@ impl SymbolState {
};

std::mem::swap(&mut a, self);
self.declarations
.live_declarations
.union(&b.declarations.live_declarations);

let mut a_defs_iter = a.bindings.live_bindings.iter();
let mut b_defs_iter = b.bindings.live_bindings.iter();
Expand Down Expand Up @@ -449,10 +452,8 @@ impl SymbolState {
let mut opt_a_decl: Option<u32> = a_decls_iter.next();
let mut opt_b_decl: Option<u32> = b_decls_iter.next();

let push = |decl,
vis_constraints_iter: &mut VisibilityConstraintsIntoIterator,
let push = |vis_constraints_iter: &mut VisibilityConstraintsIntoIterator,
merged: &mut Self| {
merged.declarations.live_declarations.insert(decl);
let vis_constraints = vis_constraints_iter
.next()
.expect("declarations and visibility_constraints length mismatch");
Expand All @@ -466,15 +467,15 @@ impl SymbolState {
match (opt_a_decl, opt_b_decl) {
(Some(a_decl), Some(b_decl)) => match a_decl.cmp(&b_decl) {
std::cmp::Ordering::Less => {
push(a_decl, &mut a_vis_constraints_iter, self);
push(&mut a_vis_constraints_iter, self);
opt_a_decl = a_decls_iter.next();
}
std::cmp::Ordering::Greater => {
push(b_decl, &mut b_vis_constraints_iter, self);
push(&mut b_vis_constraints_iter, self);
opt_b_decl = b_decls_iter.next();
}
std::cmp::Ordering::Equal => {
push(a_decl, &mut b_vis_constraints_iter, self);
push(&mut b_vis_constraints_iter, self);

let a_vis_constraint = a_vis_constraints_iter
.next()
Expand All @@ -487,12 +488,12 @@ impl SymbolState {
opt_b_decl = b_decls_iter.next();
}
},
(Some(a_decl), None) => {
push(a_decl, &mut a_vis_constraints_iter, self);
(Some(_), None) => {
push(&mut a_vis_constraints_iter, self);
opt_a_decl = a_decls_iter.next();
}
(None, Some(b_decl)) => {
push(b_decl, &mut b_vis_constraints_iter, self);
(None, Some(_)) => {
push(&mut b_vis_constraints_iter, self);
opt_b_decl = b_decls_iter.next();
}
(None, None) => break,
Expand Down
Loading