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

Implement `AsHandle` and `AsHandleMut` traits. #320

Closed
wants to merge 2 commits into from

Conversation

@ejpbruel
Copy link
Contributor

ejpbruel commented Nov 22, 2016

Several types in rust-mozjs, such as Heap, PersistentRooted, and RootedGuard, represent a rooted
value. A common property of rooted values is that a handle can be obtained to it. Rather than
implement this property separately for each of these types, we should abstract it out into these
two traits.

This has several advantages: for instance, one can write functions that take any rooted value as
argument, regardless of how it is implemented, and then obtain a handle to it. Additionally, it
allows one to implement these traits for newtypes which underlying type is a rooted value.

This change is Reviewable

Rust-mozjs currently does not define any methods to create instances of HandleValueArray.
Consequently, consumers have to create these instances manually. This is unnecessary boilerplate,
and therefore should be abstracted behind a function.
@ejpbruel ejpbruel force-pushed the ejpbruel:ashandle branch 4 times, most recently from 2865d20 to 63a53f0 Nov 22, 2016
Several types in rust-mozjs, such as Heap, PersistentRooted, and RootedGuard, represent a rooted
value. A common property of rooted values is that a handle can be obtained to it. Rather than
implement this property separately for each of these types, we should abstract it out into these
two traits.

This has several advantages: for instance, one can write functions that take any rooted value as
argument, regardless of how it is implemented, and then obtain a handle to it. Additionally, it
allows one to implement these traits for newtypes which underlying type is a rooted value.
@ejpbruel ejpbruel force-pushed the ejpbruel:ashandle branch from 63a53f0 to 3779cdc Nov 22, 2016
Copy link
Member

jdm left a comment

I'm generally neutral on these changes, since it will require importing another trait for fairly common functionality. I'm also concerned about the safety of the new Heap::set method.

let ptr = self.ptr.get();
let prev = *ptr;
*ptr = v;
T::post_barrier(ptr, prev, v);

This comment has been minimized.

@jdm

jdm Nov 22, 2016

Member

Why is it safe to avoid this post barrier?

/// Given the types `$T` and `$U`, implements the trait `AsHandleMut<$T>` for the type `$U`, where
/// `$U` is a newtype whose underlying type implements `HandleMut<$T>`.
#[macro_export]
macro_rules! derive_as_handle_mut {

This comment has been minimized.

@jdm

jdm Nov 22, 2016

Member

I don't really see a purpose for defining these macros here. The implementations aren't hard to write by hand, and I haven't come up with a use case for this in Servo yet.

@jimblandy
Copy link

jimblandy commented Nov 23, 2016

I suggested AsHandle and AsHandleMut because they would help write generic code that could take a reference to any value that could provide a handle, rather than making the callers extract the handles themselves. It seems analogous to the way the std::fs functions accept any argument that implements AsRef<Path>.

@bors-servo
Copy link
Contributor

bors-servo commented Jan 27, 2017

The latest upstream changes (presumably #328) made this pull request unmergeable. Please resolve the merge conflicts.

@jdm
Copy link
Member

jdm commented Jan 27, 2017

Closing due to lack of activity.

@jdm jdm closed this Jan 27, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.