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.

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

region_infer: BitMatrix representation of region values #16

Merged

Conversation

zackmdavis
Copy link

@nikomatsakis

This should be more efficient than allocating two BTreeSets for every region variable?—as it is written in rust-lang#45670.

This isn't addressing the question of auxillary methods for BitMatrix. I'm also not immediately sure what kind of testing is appropriate.

Copy link
Owner

@nikomatsakis nikomatsakis left a comment

Choose a reason for hiding this comment

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

This looks good to me! I left a few nits. I'll merge tomorrow -- if you don't get around to fixing the nits by then, I'll just do it myself as I merge I think.


// Find every region `o` such that `fr: o`
// (because `fr` includes `end(o)`).
for &outlived_fr in &fr_value.free_regions {
for outlived_fr in fr_value.filter(|&i| i < self.num_free_regions) {
Copy link
Owner

Choose a reason for hiding this comment

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

I think take_while would be better -- it would stop the loop sooner

for &fr in &from_region.free_regions {
changed |= to_region.free_regions.insert(fr);
let free_region_indices = inferred_values.iter(from_region.index())
.filter(|&i| i < self.num_free_regions).collect::<Vec<_>>();
Copy link
Owner

Choose a reason for hiding this comment

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

I think take_while would be more efficient


/// The constraints we have accumulated and used during solving.
constraints: Vec<Constraint>,

/// A map from each MIR Location to its column index in
/// `liveness_constraints`/`inferred_values`
point_indices: BTreeMap<Location, usize>,
Copy link
Owner

Choose a reason for hiding this comment

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

It'd be nice to expand this comment to explain that the first N indices are the free regions -- or at least comment on that somewhere.

@nikomatsakis
Copy link
Owner

Thanks @zackmdavis =)

@zackmdavis
Copy link
Author

(force-push 879feae for .take_while and brief free-region-column commentary)

This should be more efficient than allocating two BTreeSets for every
region variable?—as it is written in rust-lang#45670.
@nikomatsakis
Copy link
Owner

Warning: rebased!

@nikomatsakis nikomatsakis merged commit 466e135 into nikomatsakis:nll-master Nov 22, 2017
@nikomatsakis
Copy link
Owner

And merged.

@zackmdavis zackmdavis deleted the nll_bitmatrix_region branch January 13, 2018 07:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants