-
Notifications
You must be signed in to change notification settings - Fork 286
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
neon: Relax the lifetime constraints on TypedArray
borrows
#877
Conversation
@@ -22,43 +22,40 @@ pub trait TypedArray: private::Sealed { | |||
/// | |||
/// This may not be used if a mutable borrow is in scope. For the dynamically | |||
/// checked variant see [`TypedArray::try_borrow`]. | |||
fn as_slice<'a: 'b, 'b, C>(&'b self, cx: &'b C) -> &'b [Self::Item] | |||
fn as_slice<'cx, 'a, C>(&self, cx: &'a C) -> &'a [Self::Item] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'a
is renamed to 'cx
across the board for clarity. 'b
becomes 'a
since that's the only lifetime we need.
|
||
/// Statically checked mutable borrow of binary data. | ||
/// | ||
/// This may not be used if any other borrow is in scope. For the dynamically | ||
/// checked variant see [`TypedArray::try_borrow_mut`]. | ||
fn as_mut_slice<'a: 'b, 'b, C>(&'b mut self, cx: &'b mut C) -> &'b mut [Self::Item] | ||
fn as_mut_slice<'cx, 'a, C>(&mut self, cx: &'a mut C) -> &'a mut [Self::Item] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The 'a: 'b
bound was unnecessarily complicated. It's not possible to have references that don't meet this requirement.
&self, | ||
lock: &'b Lock<'b, C>, | ||
) -> Result<Ref<'b, Self::Item>, BorrowError> | ||
fn try_borrow<'cx, 'a, C>(&self, lock: &'a Lock<C>) -> Result<Ref<'a, Self::Item>, BorrowError> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Elided the inner lifetime of Lock
to simplify.
} | ||
|
||
if let Ok(v) = v.downcast::<JsBuffer, _>(cx) { | ||
return Ok(Cow::Borrowed(v.as_slice(cx))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the primary thing we are trying to allow; returning a reference to a handle that was downcast.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is really cool!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like I should have some suggestion, but borrowck did all the heavy lifting for me :P
LGTM!
} | ||
|
||
if let Ok(v) = v.downcast::<JsBuffer, _>(cx) { | ||
return Ok(Cow::Borrowed(v.as_slice(cx))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is really cool!
The current lifetimes are overly restrictive. The reference is valid for as long as the handle is valid. Since the `Context` is always the most narrow scope, it will be valid for at *least* as long as `Context`, even if the `Handle` type is dropped. This allows downcasting and returning a borrow within the same function.
3cb3b81
to
112e773
Compare
The current lifetimes are overly restrictive. The reference is valid for as long as the handle is valid. Since the
Context
is always the most narrow scope, it will be valid for at least as long asContext
, even if theHandle
type is dropped. This allows downcasting and returning a borrow within the same function.See this thread for more details. https://rust-bindings.slack.com/archives/C0HE1SA65/p1647378845225649