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

Store the JSContext in TLS for easy access. #322

Merged
merged 1 commit into from Dec 22, 2016
Merged

Store the JSContext in TLS for easy access. #322

merged 1 commit into from Dec 22, 2016

Conversation

@Ms2ger
Copy link
Collaborator

Ms2ger commented Dec 20, 2016 •
edited by larsbergstrom

This change is Reviewable
@Ms2ger Ms2ger mentioned this pull request Dec 21, 2016
Merged
@nox
Copy link
Member

nox commented Dec 21, 2016

Why not put the TLS thing in servo/servo? Why forbid the creation of two unrelated runtimes on a single thread?
@fitzgen
Copy link
Member

fitzgen commented Dec 21, 2016

Why forbid the creation of two unrelated runtimes on a single thread?

SpiderMonkey requires a 1:1 relationship between a JSRuntime and its thread.
@nox
Copy link
Member

nox commented Dec 21, 2016

@fitzgen Thanks!

@Ms2ger Could you make ::get() panic if the pointer is null?
@Ms2ger
Store the JSContext in TLS for easy access.
Loading status checks…
c8ebc6d
@Ms2ger Ms2ger force-pushed the tls-runtime branch from 19f9b25 to c8ebc6d Dec 22, 2016
@nox
Copy link
Member

nox commented Dec 22, 2016

@bors-servo r+
@bors-servo
Copy link
Contributor

bors-servo commented Dec 22, 2016

📌 Commit c8ebc6d has been approved by nox
@bors-servo
Copy link
Contributor

bors-servo commented Dec 22, 2016

⌛ Testing commit c8ebc6d with merge 1941f1c...
bors-servo added a commit that referenced this pull request Dec 22, 2016
@bors-servo
Auto merge of #322 - servo:tls-runtime, r=nox
Loading status checks…
1941f1c 
Store the JSContext in TLS for easy access.

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

bors-servo commented Dec 22, 2016

☀️ Test successful - status-appveyor, status-travis
@bors-servo bors-servo merged commit c8ebc6d into master Dec 22, 2016
4 checks passed
4 checks passed
continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
homu Test successful
Details
@nox nox deleted the tls-runtime branch Dec 22, 2016
@nox
Copy link
Member

nox commented Dec 23, 2016

I still believe that was the wrong way to go, because now if we want to use the Runtime value and Runtime::get to do safe abstractions around for example ToJsValConvertible, we still won't be able to avoid some unsafe code in servo for dereferencing the result of Runtime::get, whereas we could make it be owned by TLS if it were done in servo instead of here.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
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

4 participants
@Ms2ger @nox @fitzgen @bors-servo
You can’t perform that action at this time.