Skip to content

Simplify lifetimes #19

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

felixwrt
Copy link

Hi,

looking through this crate, I noticed that there were lots of lifetime parameters, some of which aren't useful IMO. In this MR, I'm proposing a refactoring to simplify the lifetimes while also making the crate more flexible:

There are several types in this crate that contain a DevTree, which is a wrapper around &[u8]. These types currently contain a reference to the devtree, for example fdt: &'a DevTree<'dt>. This makes it a double reference (&'a &'dt [u8] basically) and the outer reference doesn't provide significant value. I've therefore replaced occurrences of &'a DevTree<'dt> in several types with DevTree<'dt> (changing the attribute from being a reference to being a value). Here's a diff of one of the types I've changed:

-pub struct DevTreeParseIter<'r, 'dt: 'r> {
+pub struct DevTreeParseIter<'dt> {
     pub offset: usize,
-    pub fdt: &'r DevTree<'dt>,
+    pub fdt: DevTree<'dt>,
 }

DevTree has the memory layout of a slice, which is a fat pointer (pointer + length). Storing it by value instead of by reference makes the type containing it slightly larger, but also removes one level of indirection (which is good for cache locality).

The other (more important) effect of this change is that it removes a lifetime parameter from lots of definitions. It also makes the APIs more flexible:

Currently, one needs to create an instance of DevTree which must outlive all iterators created from it. Something like this currently doesn't compile:

#[test]
fn test() {
    let data = [1,2,3, /* ... */];
    let mut iter = {
        let blob = unsafe { DevTree::new(&data).unwrap() };
        DevTreeNodeIter(DevTreeIter::new(&blob))
        //                                ^^^^^ borrowed value does not live long enough
    };

    while let Some(x) = iter.next().unwrap() {
        dbg!(x.name().unwrap());
    }
}

With the proposed refactoring, this works fine:

#[test]
fn test() {
    let data = [1,2,3, /* ... */];
    let mut iter = {
        let blob = unsafe { DevTree::new(&data).unwrap() };
        DevTreeNodeIter(DevTreeIter::new(blob))
    };

    while let Some(x) = iter.next().unwrap() {
        dbg!(x.name().unwrap());
    }
}

DevTree isn't required to outlive DevTreeIter anymore, which is more flexible. The remaining 'dt lifetime makes sure that the data outlives DevTreeIter. Putting the data into the inner scope in the exmple above like this won't compile:

#[test]
fn test() {
    let mut iter = {
        let data = [1,2,3, /* ... */];
        let blob = unsafe { DevTree::new(&data).unwrap() };
        //                               ^^^^^ borrowed value does not live long enough
        DevTreeNodeIter(DevTreeIter::new(blob))
    };

    while let Some(x) = iter.next().unwrap() {
        dbg!(x.name().unwrap());
    }
}

Storing DevTree by value meant that I had to clone it in a couple of places. That's not an issue though IMO as it simply copies the fat pointer (16 bytes on a 64 bit system).

Let me know if you have questions regarding this MR.

Looking forward to hearing from you!

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.

1 participant