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

Add the Entry API to Option<T> #1278

Closed
wesleywiser opened this issue Sep 11, 2015 · 3 comments
Closed

Add the Entry API to Option<T> #1278

wesleywiser opened this issue Sep 11, 2015 · 3 comments

Comments

@wesleywiser
Copy link
Member

Motivation

While working on a project recently, I needed to optimize some code by performing an expensive operation lazily. The change looked something like this:

Before:

pub struct ImageCell {
    pub path: PathBuf,
    pub image_stats: Metadata //this is really expensive to compute
}

After:

pub struct ImageCell {
    pub path: PathBuf,
    image_stats: Option<Metadata> //delay loading this until it's needed
}

impl ImageCell {
    pub fn get_image_stats(&mut self) -> Metadata {
        if let Some(metadata) = self.image_stats {
            return metadata.clone();
        }

        //this is the expensive calculation
        let metadata = calculate_metadata(self.path);
        let return_metadata = metadata.clone();

        self.image_stats = Some(metadata);
        return_metadata
    }
}

Initially, I tried to write get_image_stats like this:

pub fn get_image_stats(&mut self) -> Metadata {
    match self.image_stats {
        Some(metadata) => metadata.clone(),
        None => {
            let metadata = calculate_metadata(self.path);
            let return_metadata = metadata.clone();
            self.image_stats = Some(metadata);
            return_metadata
        }
    }
}

But I ran into borrow errors because the match had an outstanding borrow when I tried to assign to self.image_stats. After thinking about it and researching similar issues on StackOverflow, I came up with the above code which works. Thinking about it some more lead me to realize that this is similar to the Entry API. If there was an API like that for Option, then the implementation of get_image_stats could be written like this:

pub fn get_image_stats(&mut self) -> Metadata {
    self.image_stats.or_insert_with(|| calculate_metadata(self.path)).clone()
}

I think this is a lot nicer than either of my tries.

Implementation

I pretty shamelessly ripped this off of the existing Entry API. There's probably a better way to write some of this; I'm still getting the hang of Rust.

pub enum Entry<'a, T: 'a> {
    Vacant(VacantEntry<'a, T>),

    Occupied(OccupiedEntry<'a, T>)
}

pub struct VacantEntry<'a, T: 'a> {
    option: &'a mut Option<T>
}

pub struct OccupiedEntry<'a, T: 'a> {
    option: &'a mut Option<T>
}

impl<T> Option<T> {
    fn entry(&mut self) -> Entry<T> {
        match *self {
            None => Vacant(VacantEntry { option: self }),
            Some(_) => Occupied(OccupiedEntry { option: self })
        }
    }
}

impl<'a, T> Entry<'a, T> {
    pub fn or_insert(self, default: T) -> &'a mut T {
        match self {
            Occupied(entry) => entry.into_mut(),
            Vacant(entry) => entry.insert(default)
        }
    }

    pub fn or_insert_with<F: FnOnce() -> T>(self, default: F) -> &'a mut T {
        match self {
            Occupied(entry) => entry.into_mut(),
            Vacant(entry) => entry.insert(default())
        }
    }
}

impl<'a, T> VacantEntry<'a, T> {
    pub fn insert(self, value: T) -> &'a mut T {
        mem::replace(self.option, Some(value));

        self.option.as_mut().unwrap()
    }
}

impl<'a, T> OccupiedEntry<'a, T> {
    pub fn into_mut(self) -> &'a mut T {
        self.option.as_mut().unwrap()
    }
}
@Diggsey
Copy link
Contributor

Diggsey commented Sep 11, 2015

@wesleywiser I think you may be mis-interpretting the error messages? None of your examples (including the "working" solution) will compile as-is, but with a couple of modifications they all work fine: http://is.gd/Ru5Akw

Adding an entry-style API for Option is unnecessary: you can just add useful methods directly on Option itself. For example, it already has a take method (the equivalent of remove in the entry API). I could see the usefulness of an or_put method though.

@Gankra
Copy link
Contributor

Gankra commented Sep 11, 2015

Literally all the entry API does is save on the cost of a double lookup, which is Not A Thing for Option

@wesleywiser
Copy link
Member Author

@Diggsey Interesting... Yes, you're right I misunderstood the error. Thanks for the explanation!

I like the idea of an or_put method on Option. Should I close this issue and open another for that?

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

No branches or pull requests

3 participants