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

fix ABI incompatibility problem of some JS direct-connected API #398

Closed
wants to merge 1 commit into from

Conversation

@aydinkim
Copy link

aydinkim commented Apr 24, 2013

As you know, there is a issue when connect rust to c(++) between another ABIs.
So I added some glue code to use JS API normally regardless of ABI difference.

I had also modified the module : rust-mozjs
actual commit is on my local github.
aydinkim/rust-mozjs@35c7752

and the source code diff is attached below.
https://gist.github.com/aydinkim/5450506

@jdm
Copy link
Member

jdm commented Apr 24, 2013

I'm willing to commit this for now (after a rebase that clears up the new places that also use JS_GetReservedSlot), but these adhoc changes worry me. We need to sit down and figure out whether we need to wrap all of the relevant JSAPI calls or find some other solution.

@metajack
Copy link
Contributor

metajack commented May 6, 2013

Ping? Is this going to get rebased, or is it no longer needed?

@jdm
Copy link
Member

jdm commented May 13, 2013

We discussed this today, and we believe that a much less intrusive fix is possible. If we define JSVal as a struct containing a u64 in jsapi.rs, that should cause rustc to use the correct calling convention on ARM. Could you try that fix instead?

@ILyoan
Copy link
Contributor

ILyoan commented May 14, 2013

@jdm What a clever and neat idea!!!
I'll try with this idea.

@yichoi
Copy link
Contributor

yichoi commented May 14, 2013

We have been thinking 2 transitional ideas: one is wrapping with struct like jdm mentioned and the other is a kind of unsafe enum. I agree to struct approach for this issue but I want to know whether there is really general solution on rust side or not.

@ILyoan
Copy link
Contributor

ILyoan commented May 14, 2013

I'm working on this on my rust-mozjs branch and servo branch.
Looks working on x86 linux, but not tested on android yet.
@jdm @yichoi Could you confirm if this is right direction?
If right, I'll rebase it and open PR after huge change from pcwalton landed.

@jdm
Copy link
Member

jdm commented May 14, 2013

Yes, that looks right to me.

@yichoi
Copy link
Contributor

yichoi commented May 15, 2013

+1

@ILyoan
Copy link
Contributor

ILyoan commented May 22, 2013

@jdm thinks you for your comments.
I have a patch for this issue but couldn't test it on android device yet.
Close this PR. I'll open another PR after test it on android.

@ILyoan ILyoan closed this May 22, 2013
ChrisParis pushed a commit to ChrisParis/servo that referenced this pull request Sep 7, 2014
Add more tests for Proximity Events
glennw pushed a commit to glennw/servo that referenced this pull request Jan 16, 2017
Implement GetShaderInfoLog, GetProgramInfoLog and ValidateProgram

<!-- Reviewable:start -->
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/398)
<!-- Reviewable:end -->
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

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