Skip to content

rust: enhance PointerWrapper. - #386

Merged
wedsonaf merged 1 commit into
Rust-for-Linux:rustfrom
wedsonaf:borrowed
Jun 23, 2021
Merged

rust: enhance PointerWrapper.#386
wedsonaf merged 1 commit into
Rust-for-Linux:rustfrom
wedsonaf:borrowed

Conversation

@wedsonaf

@wedsonafwedsonaf commented Jun 22, 2021

Copy link
Copy Markdown

This formalises how wrapped values can be accessed after they've been
converted to 'pointers'.

In upcoming PRs, we will have Box<T> instances access &T, but
Ref<T> will provide access to &Ref<T> so that we can increment the
refcount when needed.

Signed-off-by: Wedson Almeida Filho wedsonaf@google.com

@ksquirrel

This comment has been minimized.

Sign in to view
@wedsonafwedsonaf mentioned this pull request Jun 22, 2021
Merged
TheSven73
TheSven73 reviewed Jun 22, 2021
View reviewed changes

@TheSven73TheSven73 left a comment •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I like this a lot !

(edit: I see there is a full explanation already in #387, so all good)

Comment threadrust/kernel/of.rs
Comment threadrust/kernel/types.rs Outdated
Comment threadrust/kernel/types.rs
Comment threadrust/kernel/types.rs
Comment threadrust/kernel/types.rs Outdated
@ksquirrel

This comment has been minimized.

Sign in to view
@wedsonaf

wedsonaf commented Jun 23, 2021•
edited
Loading

Copy link
Copy Markdown
Author

v1 -> v2

  • Rebase
  • Use cast()
  • Rename UnsafeBorrow to UnsafeReference

TheSven73
TheSven73 reviewed Jun 23, 2021
View reviewed changes
Comment threadrust/kernel/types.rs Outdated
// so it is safe to dereference the raw pointer.
// The safety requirements also ensure that the object remains alive for the lifetime of
// the returned value.
unsafe { UnsafeReference::new(&*(ptr as *const T)) }

@TheSven73TheSven73Jun 23, 2021

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Nit: left out a cast() here?

@wedsonafwedsonafJun 23, 2021

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Oops. I had missed this one because it goes away when I update Ref's implementation.

Anyway, fixed it now. PTAL.

@wedsonaf
rust: enhance PointerWrapper.
20bf0d4
This formalises how wrapped values can be accessed after they've been
converted to 'pointers'.
In upcoming PRs, we will have `Box<T>` instances access `&T`, but
`Ref<T>` will provide access to `&Ref<T>` so that we can increment the
refcount when needed.
Signed-off-by: Wedson Almeida Filho <wedsonaf@google.com>
@ksquirrel

ksquirrel commented Jun 23, 2021

Copy link
Copy Markdown
Member

Review of 20bf0d4fb254:

TheSven73
TheSven73 approved these changes Jun 23, 2021
View reviewed changes

@TheSven73TheSven73 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM !

TheSven73
TheSven73 reviewed Jun 23, 2021
View reviewed changes
Comment threadrust/kernel/types.rs
///
/// `ptr` must have been returned by a previous call to [`PointerWrapper::into_pointer`].
/// Additionally, [`PointerWrapper::from_pointer`] can only be called after *all* values
/// returned by [`PointerWrapper::borrow`] have been dropped.

@TheSven73TheSven73Jun 23, 2021

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Are there a couple of safety constraints we've omitted here?
Following these # Safety instructions, are we allowed to:

  • call from_pointer() multiple times on the same pointer?
  • call borrow() after the call to from_pointer() ?

If so, does this merit a follow-up PR?

@wedsonafwedsonafJun 23, 2021

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Yes, this should be clarified. I'll prepare a follow-up PR.

@wedsonaf
wedsonaf merged commit 3c47616 into Rust-for-Linux:rustJun 23, 2021
@wedsonaf
wedsonaf deleted the borrowed branch June 23, 2021 13:14
@adamrkadamrk mentioned this pull request Jun 29, 2021
Closed
@wedsonafwedsonaf mentioned this pull request Jul 13, 2021
Draft
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nbdd0121nbdd0121nbdd0121 left review comments
+2 more reviewers
@TheSven73TheSven73TheSven73 approved these changes
@bjorn3bjorn3bjorn3 left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@wedsonaf@ksquirrel@TheSven73@nbdd0121@bjorn3