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

C++ rewrite #25

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

C++ rewrite #25

wants to merge 2 commits into from

Conversation

dflemstr
Copy link
Owner

No description provided.

@jeromegn
Copy link

That's very interesting. Are you removing the "glue" code entirely because it won't be needed or is this branch still in flux (I assume so based on the last commit message.)

I'm glad to see activity here!

@dflemstr
Copy link
Owner Author

Yes, I'm trying to make the library not need any glue code, and then eventually maybe support several V8 versions with #[cfg()] since then it becomes a lot easier. Very much work in progress though!

@piaoger
Copy link

piaoger commented Apr 22, 2018

nice to see the activity too 👍

@jeromegn
Copy link

@dflemstr Out of curiosity, which v8 version are you using with this branch?

I know this is a WIP, but I thought I'd see where things are at. It's actually pretty cool, the build process seems to work fine for me, it's just some of the src/ files are outdated or using the wrong code (maybe due to my v8 version: 6.3)

We're heavily invested in v8, the simplest way to run it for us has been using node.js, but we'd love to use Rust.

@dflemstr
Copy link
Owner Author

@jeromegn Right now I'm targeting 6.8.50-1 (arbitrary) although as you can see I'm not actively spending too much time on this code for now due to its complexity.

If you would like to help me out with this, I could open up the branch to external collaboration...

@jeromegn
Copy link

I'd love to help, but I've only just started using both Rust and C++. I don't think I'd be much help. (see my very newbie question #24 as proof)

I've been wondering if it's even worth generating the bindings like that. A wrapper like you have in master probably works just fine. It's very outdated, but it could be updated. In general, changes v8 aren't that significant when they bump versions, at least not for the embedders. I'm trying to use 6.3 right now (because it's pre-built by the libv8 gem) and I think the ArrayBuffer Allocator changed, breaking the implementation here.

Is it possible to generate bindings with no custom C code? I see there is stil some C in this branch. Doesn't Rust provide a zero-cost abstraction to calling C code?

@jeromegn
Copy link

Ok, I've learned a ton about C++, V8 and Rust in the past months and I'd like to contribute.

There are a lot of issues while trying to build this right now. A lot are probably because of deprecated rust things in more recent nightlies.

If you have a list of what needs to be done, I can start tackling things.

There are good SpiderMonkey bindings: https://github.com/servo/mozjs which seems to use a technique similar to what's in master. Which make me think it might be worth updating master to use a more recent V8 instead of a full blown rewrite.

@Neurrone
Copy link

Neurrone commented Sep 2, 2018

Hi,

If anyone's interested, I'm attempting to get this working with V8 7.0 on this repo.

For v8-sys, I'm relying entirely on Bindgen for bindings, and use the default platform implementation on V8 so I don't have to use any C++. To simplify the build script, I'm also relying on the user having built static libs, and putting them in a place where the build script can find them.

I'm using much of the existing code in the V8 crate, modified where appropriate.

I just started this yesterday, and unfortunately it's currently crashing when attempting to create a new context for some reason, using the same context.rs code as this branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants