Skip to content

/ rust-mozjs

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 a conversion from the newly-#[stable] `ptr::NonNull` type #390

Merged
merged 1 commit into from Jan 23, 2018
Merged

Add a conversion from the newly-#[stable] `ptr::NonNull` type #390

merged 1 commit into from Jan 23, 2018

Conversation

@SimonSapin
Copy link
Member

SimonSapin commented Jan 21, 2018 •
edited by larsbergstrom

This will allow removing the NonNullJSObjectPtr hack in Servo, but raises the minimum Rust version to 1.25 (currently Nigthly)

This change is Reviewable
@SimonSapin
Copy link
Member Author

SimonSapin commented Jan 21, 2018 •
edited

@fitzgen @jdm @nox How do you feel about this repository requiring Rust 1.15 1.25? According to https://wiki.mozilla.org/Rust_Update_Policy_for_Firefox Gecko can use that version from April 12, but as far as I understand it doesn’t use this repo anyway.
@nox
Copy link
Member

nox commented Jan 21, 2018

I assume you meant 1.25? I don't mind.
@SimonSapin
Copy link
Member Author

SimonSapin commented Jan 21, 2018

Oops, yes. I mostly ask in case it interferes with plans to unify this repo with js/ in mozilla-central.
@SimonSapin
Copy link
Member Author

SimonSapin commented Jan 21, 2018

In particular: this is only clean-up, so if it requires adding a Cargo feature somewhere it’s not worth it and we can just wait.
@emilio
emilio reviewed Jan 21, 2018
View changes
src/conversions.rs Outdated
impl ToJSValConvertible for ptr::NonNull<JSObject> {
#[inline]
unsafe fn to_jsval(&self, cx: *mut JSContext, rval: MutableHandleValue) {
rval.set(ObjectOrNullValue(self.get()));

This comment has been minimized.

Sign in to view
@emilio

emilio Jan 21, 2018

Member

This can probably use ObjectValue and maybe_wrap_object_value.

This comment has been minimized.

Sign in to view
@SimonSapin

SimonSapin Jan 21, 2018

Author Member

Done. Indeed, that’s what the equivalent impl in Servo does.
@SimonSapin
Add a conversion from the newly-#[stable] `ptr::NonNull` type
Loading status checks…
5a68234 
This will allow removing the `NonNullJSObjectPtr` hack in Servo,
but raises the minimum Rust version to 1.25 (currently Nigthly)
@SimonSapin SimonSapin force-pushed the nonnull branch from 006ec9b to 5a68234 Jan 21, 2018
@jdm
jdm approved these changes Jan 21, 2018
View changes
@SimonSapin
Copy link
Member Author

SimonSapin commented Jan 22, 2018

I take it "approved" means:

@bors-servo r=jdm
@bors-servo
Copy link
Contributor

bors-servo commented Jan 22, 2018

📌 Commit 5a68234 has been approved by jdm
@bors-servo
Copy link
Contributor

bors-servo commented Jan 22, 2018

⌛ Testing commit 5a68234 with merge 8f5cf86...
bors-servo added a commit that referenced this pull request Jan 22, 2018
@bors-servo
Auto merge of #390 - servo:nonnull, r=jdm
Loading status checks…
8f5cf86 
Add a conversion from the newly-#[stable] `ptr::NonNull` type

This will allow removing the `NonNullJSObjectPtr` hack in Servo, but raises the minimum Rust version to 1.25 (currently Nigthly)

<!-- 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/390)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Jan 22, 2018

💥 Test timed out
@KiChjang
Copy link
Member

KiChjang commented Jan 22, 2018

@bors-servo retry
@bors-servo
Copy link
Contributor

bors-servo commented Jan 22, 2018

⌛ Testing commit 5a68234 with merge d5aabe3...
bors-servo added a commit that referenced this pull request Jan 22, 2018
@bors-servo
Auto merge of #390 - servo:nonnull, r=jdm
Loading status checks…
d5aabe3 
Add a conversion from the newly-#[stable] `ptr::NonNull` type

This will allow removing the `NonNullJSObjectPtr` hack in Servo, but raises the minimum Rust version to 1.25 (currently Nigthly)

<!-- 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/390)
<!-- Reviewable:end -->
@SimonSapin
Copy link
Member Author

SimonSapin commented Jan 23, 2018

Eh, OS X job on Travis-CI still not scheduled after 13 hours…
@bors-servo
Copy link
Contributor

bors-servo commented Jan 23, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: jdm
Pushing d5aabe3 to master...
@bors-servo bors-servo merged commit 5a68234 into master Jan 23, 2018
4 of 5 checks passed
4 of 5 checks passed
continuous-integration/travis-ci/push The Travis CI build could not complete due to an error
Details
continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@SimonSapin SimonSapin deleted the nonnull branch Jan 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@emilio emilio

@jdm jdm

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
@SimonSapin @nox @bors-servo @KiChjang @jdm @emilio
You can’t perform that action at this time.