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 VolatileRef::borrow and VolatileRef::borrow_mut #46

Merged
merged 1 commit into from
Apr 21, 2024

Conversation

mkroening
Copy link
Member

These methods are useful when you want to borrow a VolatileRef but want to avoid the additional indirection and lifetime of &mut VolatileRef<'a, T> by creating a VolatileRef<'_, T> instead.

borrow and borrow_mut mirror as_ptr and as_mut_ptr respectively, but yield volatile references instead of pointers.

Copy link
Member

@phil-opp phil-opp left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! These methods are definitely useful.

Maybe reborrow is a better name? I think the name borrow is associated with creating a & or &mut reference already. Also, there is the core::borrow::Borrow trait that returns a &Borrowed reference too.

Also, could you add a note to the docs that the methods create a VolatileRef with a shorter lifetime? Just to make it clearer how the methods differ from the identity function and why they are useful.

Signed-off-by: Martin Kröning <martin.kroening@eonerc.rwth-aachen.de>
@mkroening
Copy link
Member Author

mkroening commented Apr 20, 2024

Maybe reborrow is a better name? I think the name borrow is associated with creating a & or &mut reference already. Also, there is the core::borrow::Borrow trait that returns a &Borrowed reference too.

I intentionally drew parallels to core::borrow::Borrow here. Implementations of Borrow for &T and &mut T, as well as the implementation of BorrowMut for &mut T do exactly the kind of reborrowing, that we are doing here. The only reason, I could not implement the trait instead, is that the trait returns &T and &mut T respectively. Since VolatileRef<'_, T> is just another reference type, I think the current names are quite fitting.

If you disagree, I can change the name, though.

Edit: I think this is similar to how we use as_ptr and as_mut_ptr to refer to VolatilePtr instead of *.

Also, could you add a note to the docs that the methods create a VolatileRef with a shorter lifetime? Just to make it clearer how the methods differ from the identity function and why they are useful.

I amended the commit, although I think it can still be improved. I struggle with making this concise but also not too confusing.

Copy link
Member

@phil-opp phil-opp left a comment

Choose a reason for hiding this comment

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

Thanks for the doc updates, looks good!

Good point about the Borrow impl for &T, this is indeed quite similar, given that we're also providing a reference-like type. Let's keep the names then!

@phil-opp phil-opp merged commit 47660af into rust-osdev:main Apr 21, 2024
5 checks passed
@mkroening mkroening deleted the borrow-ref branch April 21, 2024 15:01
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.

None yet

2 participants