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 methods to create a HandleValueArray. #319

Merged
merged 1 commit into from Nov 22, 2016

Conversation

@ejpbruel
Copy link
Contributor

ejpbruel commented Nov 21, 2016

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.


This change is Reviewable

src/rust.rs Outdated
pub fn new() -> HandleValueArray {
HandleValueArray {
length_: 0,
elements_: ptr::null_mut(),

This comment has been minimized.

@jdm

jdm Nov 21, 2016

Member

This should be ptr::null() instead.

src/rust.rs Outdated
}
}

pub fn from_slice(values: &[Value]) -> HandleValueArray {

This comment has been minimized.

@jdm

jdm Nov 21, 2016

Member

This is not clearly safe to me - it's easy to create a vector which isn't rooted, leading to a handle that can point to things that get GCed. We should either remove this constructor, figure out a way to make it safe, or mark it unsafe.

This comment has been minimized.

@ejpbruel

ejpbruel Nov 21, 2016

Author Contributor

Fair enough. Having this constructor is something I depend on quite often in the Rust Debugger API, so I'd prefer to keep it around.

I did try to make this function take a &[HandleValue] instead, since that would guarantee the values are rooted. Unfortunately, a HandleValue is not a pointer, but rather a pointer to a pointer, so you can't use it to create a HandleValueArray directly. Nor can we create a Vec from the &[HandleValue], since there's no way to keep that alive after this function returns.

Given the above, I'd like to opt to make this function unsafe.

This comment has been minimized.

@jdm

jdm Nov 21, 2016

Member

Unsafe and called from_rooted_slice sounds ok to me.

@ejpbruel ejpbruel force-pushed the ejpbruel:handlevaluearray branch from 6c54738 to c268b71 Nov 22, 2016
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.
@jdm
Copy link
Member

jdm commented Nov 22, 2016

@bors-servo
Copy link
Contributor

bors-servo commented Nov 22, 2016

📌 Commit c268b71 has been approved by jdm

@bors-servo
Copy link
Contributor

bors-servo commented Nov 22, 2016

Testing commit c268b71 with merge 8c124e7...

bors-servo added a commit that referenced this pull request Nov 22, 2016
Add methods to create a HandleValueArray.

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.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/rust-mozjs/319)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Nov 22, 2016

☀️ Test successful - status-appveyor, status-travis

@bors-servo bors-servo merged commit c268b71 into servo:master Nov 22, 2016
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details
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

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