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 support for CustomAutoRooter #225

Merged
merged 1 commit into from Dec 16, 2015

Conversation

Projects
None yet
3 participants
@michaelwu
Contributor

michaelwu commented Dec 16, 2015

As requested by @jdm . Fixes servo/servo#7676. Not particularly great to use but it works for me.

Review on Reviewable

src/rust.rs Outdated
pub fn new_with_addr(cx: *mut JSContext, vftable: &'static _vftable_CustomAutoRooter, addr: *const u8) -> CustomAutoRooter {
let ctxfriend: &mut ContextFriendFields = unsafe {
mem::transmute(cx)

This comment has been minimized.

@jdm

jdm Dec 16, 2015

Member

I would prefer &mut *(cx as *mut ContextFriendFields) instead of a transmute.

src/rust.rs Outdated
}
};
ctxfriend.autoGCRooters = unsafe { mem::transmute((addr as *const *const u8).offset(1)) };

This comment has been minimized.

@jdm

jdm Dec 16, 2015

Member

I'd prefer explicit casts here instead of transmute.

}
}
impl Drop for CustomAutoRooter {

This comment has been minimized.

@jdm

jdm Dec 16, 2015

Member

Bleh, another destructor on a repr(C) type :/

@jdm

This comment has been minimized.

Member

jdm commented Dec 16, 2015

I'd like to see the transmutes replaced with casts; otherwise this looks ok.

@jdm jdm self-assigned this Dec 16, 2015

@michaelwu michaelwu force-pushed the michaelwu:car branch from 453cc8b to a91d5e4 Dec 16, 2015

@michaelwu

This comment has been minimized.

Contributor

michaelwu commented Dec 16, 2015

Transmutes replaced with casts.

@jdm

This comment has been minimized.

Member

jdm commented Dec 16, 2015

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Dec 16, 2015

📌 Commit a91d5e4 has been approved by jdm

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Dec 16, 2015

⌛️ Testing commit a91d5e4 with merge 2a98c08...

bors-servo added a commit that referenced this pull request Dec 16, 2015

Auto merge of #225 - michaelwu:car, r=jdm
Add support for CustomAutoRooter

As requested by @jdm . Fixes servo/servo#7676. Not particularly great to use but it works for me.

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/rust-mozjs/225)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Dec 16, 2015

☀️ Test successful - travis

@bors-servo bors-servo merged commit a91d5e4 into servo:master Dec 16, 2015

1 check passed

homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment