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

Return a borrowed not cloned object from Env::get #74

Closed
edmorley opened this issue Sep 8, 2021 · 3 comments · Fixed by #565
Closed

Return a borrowed not cloned object from Env::get #74

edmorley opened this issue Sep 8, 2021 · 3 comments · Fixed by #565

Comments

@edmorley
Copy link
Member

edmorley commented Sep 8, 2021

I've split this review comment out of #30 so it doesn't get lost, since the PR has already been merged.

Originally posted by @hone in https://github.com/Malax/libcnb.rs/pull/30#discussion_r699634420


@Malax Isn't it usually common here to return a borrowed and not cloned from an object?

https://github.com/Malax/libcnb.rs/blob/ac8f17f957d410678b31d268548e40c483eb2ffb/libcnb/src/env.rs#L59-L62

@edmorley
Copy link
Member Author

edmorley commented Dec 9, 2021

@Malax Is this something we want to do? (It seems so given eg https://doc.rust-lang.org/std/primitive.slice.html#method.get)

@Malax
Copy link
Member

Malax commented Jan 12, 2022

It's true that it is much more common to return a borrowed value for data structures like HashMap, Vec, etc. They do it to avoid the clone because of performance reasons. But it also makes it slightly more inconvenient to use.

In this case, I value the ease of use higher, especially since we don't have an actual performance problem with this right now. I'm totally open to change the implementation to a reference, but I don't see the need to put the work in right now.

@edmorley
Copy link
Member Author

Agree about usability. Let's close this out for now :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants