Skip to content

various internal improvements#2

Draft
ahrens wants to merge 3 commits intotprodanov:masterfrom
ahrens:cleanup
Draft

various internal improvements#2
ahrens wants to merge 3 commits intotprodanov:masterfrom
ahrens:cleanup

Conversation

@ahrens
Copy link

@ahrens ahrens commented Apr 6, 2023

Hi @tprodanov thanks for adding remove_where and other changes! While I was working on remove_select, I noticed some things I thought could be improved in the codebase and wanted to see how they would pan out so I went ahead and implemented them. This PR is against the older version of the code. If you are interested in taking any/all of these changes, I'd be happy to rebase or break out any subset of the changes. Here are the major changes:

  • Distinguish between definitely-valid and potentially-invalid index values by using Option<Ix> vs Ix. In order to not increase memory footprint, Ix is changed to NonZeroU*. (Option<NonZeroU32> is 4 bytes, same as u32.) I think this is a great improvement to being able to understand the code, catch more programming errors at compile time, and it lead to many opportunities to clarify surrounding code (e.g. with match or map). However, I haven't been able to figure out how to preserve the existing API where you can create a RangeMap<X, Y, u64>; consumers would have to be updated to use RangeMap<X, Y, NonZeroU64>. Not sure how impactful this would be for consumers.
  • Add accessors for IntervalMap::nodes[]. Slightly reduces the amount of repeated code, and makes it more clear where nodes are being mutated.
  • Minimize unsafe code in swap_nodes().
  • Reformat code with rustfmt.
  • Resolve clippy nits.

Here is an example where the change is pretty mechanical, with no additional cleanup possible. But you can't forget the .defined() check and then panic at runtime :)
old:

   fn update_subtree_interval(&mut self, index: Ix) {
        let node = &self.nodes[index.get()];
        let mut subtree_interval = node.interval.clone();
        if node.left.defined() {
            subtree_interval.extend(&self.nodes[node.left.get()].subtree_interval);
        }
        if node.right.defined() {
            subtree_interval.extend(&self.nodes[node.right.get()].subtree_interval);
        }
        self.nodes[index.get()].subtree_interval = subtree_interval;
    }

new:

    fn update_subtree_interval(&mut self, index: Ix) {
        let node = self.node(index);
        let mut subtree_interval = node.interval.clone();
        if let Some(left) = node.left {
            subtree_interval.extend(&self.node(left).subtree_interval);
        }
        if let Some(right) = node.right {
            subtree_interval.extend(&self.node(right).subtree_interval);
        }
        self.node_mut(index).subtree_interval = subtree_interval;
    }

You can see the substantial changes more clearly by looking at just the last commit.

@tprodanov
Copy link
Owner

tprodanov commented Apr 7, 2023

Thank you, that is indeed very useful! I did not know about NonZeroX. I will work on these improvements.
Changing index type can be done with

pub trait IndexType: Copy + Display + Sized + Eq {
    type T: Copy + Display + Sized + Eq;

    fn new(val: usize) -> Self::T;

    fn get(val: Self::T) -> usize;
}

where T is the NonZeroX.
Nevertheless, serialization will probably need to be broken.

@ahrens
Copy link
Author

ahrens commented Apr 7, 2023

Yeah, the "niche" in NonZeroX is super useful in case you want a zero-cost Option of it.

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