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.

Sign up for GitHub

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

Jump to bottom

Remove the need for rust-mozjs to use unstable Rust features #18875

Merged
merged 12 commits into from
Oct 16, 2017
Merged

Remove the need for rust-mozjs to use unstable Rust features #18875

merged 12 commits into from
Oct 16, 2017

Conversation

SimonSapin
Copy link
Member

@SimonSapin SimonSapin commented Oct 14, 2017 •
edited

This change is Reviewable

@highfive
Copy link

Heads up! This PR modifies the following files:

  • @aneeshusa: etc/ci/buildbot_steps.yml
  • @KiChjang: components/script/dom/bindings/codegen/CodegenRust.py, components/script/dom/bindings/nonnull.rs, components/script/dom/textencoder.rs, components/script/dom/vrdisplay.rs, components/script/dom/crypto.rs and 24 more
  • @paulrouget: components/compositing/compositor.rs
  • @fitzgen: components/script/dom/bindings/codegen/CodegenRust.py, components/script/dom/bindings/nonnull.rs, components/script/dom/textencoder.rs, components/script/dom/vrdisplay.rs, components/script/dom/crypto.rs and 24 more
  • @emilio: components/script/dom/webgl_extensions/extensions.rs, components/script/dom/webgl_extensions/ext/webglvertexarrayobjectoes.rs, components/script/dom/webglrenderingcontext.rs, components/script/dom/webgl_extensions/wrapper.rs

@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label Oct 14, 2017
@highfive
Copy link

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!

SimonSapin
SimonSapin commented Oct 14, 2017
View reviewed changes
components/script/dom/bindings/codegen/CodegenRust.py Outdated
${isTypedMethod},
${slotIndex} as u16,
)
((JSJitInfo_OpType::${opType} as u8 as u32) << 0) |
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Next I’ll move this bit-shifting into a macro in rust-mozjs

@nox
Copy link
Contributor

nox commented Oct 14, 2017

I'm not really ok with the removal of NonZero without any proof that it could break in the future. We can just make a NonZeroPtr<T> that stores a &'static T or something. I'll defer to @jdm for the reviewal.

The rest looks fine to me, but it sounds like you want to remove all const functions from script, which I don't think is a good idea either, given that would require quite a bit of changes that will make the code way more uglier.

@SimonSapin
Copy link
Member Author

The default is still to use core::nonzero::NonZero with its memory layout optimization. This only changes memory layout when compiling with --no-default-features --features default-except-unstable. And I don’t expect anyone to actually use a build compiled with this mode. It only exists to enable more parts of Servo to be included in Rust’s upstream regression testing and performance testing.

I’m not planning to touch const fn’s in script. The JSJitInfo::new_bitfield_1 change is comparatively easy, and allows limiting the required unstable features to the servo/servo repo.

@SimonSapin SimonSapin mentioned this pull request Oct 14, 2017
Merged
@SimonSapin
Copy link
Member Author

Pushed a few more commits. The last one updates rust-mozjs to a commit that hasn’t been merged yet: servo/rust-mozjs#373 so this PR should not land until that one has been reviewed.

bors-servo pushed a commit to servo/rust-mozjs that referenced this pull request Oct 16, 2017
Auto merge of #373 - servo:stable, r=jdm
fdcc7a0 
Remove the use of unstable Rust features.

This modifies generated files, but http://searchfox.org/mozilla-central/source/js/rust/ already has similar changes through `rust_target(bindgen::RustTarget::Stable_1_19)`

This depends on servo/servo#18875.

<!-- 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/373)
<!-- Reviewable:end -->
@jdm
Copy link
Member

jdm commented Oct 16, 2017

This looks fine to me.

@SimonSapin
Copy link
Member Author

@bors-servo r=nox,jdm

@bors-servo
Copy link
Contributor

📌 Commit 7c5fd99 has been approved by nox,jdm

@highfive highfive assigned nox and unassigned wafflespeanut Oct 16, 2017
@highfive highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels Oct 16, 2017
@bors-servo
Copy link
Contributor

🔒 Merge conflict

@bors-servo
Copy link
Contributor

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

@highfive highfive added S-needs-rebase There are merge conflict errors. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Oct 16, 2017
@SimonSapin
Remove unused feature flag
3f46702
@SimonSapin
Give up on NonZeroUsize memory layout optimization with unstable feat…
4ee1b26 
…ures.

I’m not confident of the implications regarding variance, dropck, struct
aliasing, etc. when this is extended to generic `*const T` and `*mut T`.

Release builds should use `unstable` to enable other optimizations anyway.
@SimonSapin
Replace NonZeroU32 and NonZeroUsize with a generic NonZero
9e4865b 
The API is identical to core::nonzero::NonZero
@SimonSapin
Build with --features default-except-unstable on CI.
115d859
@SimonSapin
Replace NonZero<*mut JSObject> with a wrapper to enable local trait i…
e2fafd2 
…mpls.
@SimonSapin
Move remaining uses of NonZero to our nonzero crate
99b052d
@SimonSapin
Replace uses of libcore with libstd in components/script
a819cfb
@SimonSapin
Don’t rely on unstable 'const fn's in rust-mozjs, so we can remove them.
3bb76a5
@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label Oct 16, 2017
@SimonSapin
Copy link
Member Author

@bors-servo r=nox,jdm

@bors-servo
Copy link
Contributor

📌 Commit 49e4540 has been approved by nox,jdm

@highfive highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. S-needs-rebase There are merge conflict errors. labels Oct 16, 2017
@bors-servo
Copy link
Contributor

⌛ Testing commit 49e4540 with merge e8a6f28...

bors-servo pushed a commit that referenced this pull request Oct 16, 2017
Auto merge of #18875 - servo:stable-js, r=nox,jdm
e8a6f28 
Remove the need for rust-mozjs to use unstable Rust features

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

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev
Approved by: nox,jdm
Pushing e8a6f28 to master...

@bors-servo bors-servo merged commit 49e4540 into master Oct 16, 2017
@highfive highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Oct 16, 2017
@SimonSapin SimonSapin deleted the stable-js branch October 17, 2017 00:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@nox nox

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@SimonSapin @highfive @nox @jdm @bors-servo @wafflespeanut