-
Notifications
You must be signed in to change notification settings - Fork 188
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
Target-specific code #1655
Target-specific code #1655
Conversation
Turning into a draft until underlying PRs are merged |
@OlivierNicole, can you rebase, fix and undraft this PR |
3045d76
to
6820074
Compare
6820074
to
4a28c27
Compare
Done. |
Do we really need to pass target around ? It creates a lot of noise for few uses only. It could be turned into an global config like |
What I like about passing The readability does not seem too much impaired to me, but it’s not a strong opinion, if there’s opposition to it I’m OK with a global config parameter, since it’s not a value that is expected to change during a run. |
Looking at the diff, I find it hard to extract where the behavior depend on the target because there are so many targets passed around. I like that values are explicitly passed but I don't see a reason to treat backends differently from other "config" (e.g. use_js_string).
|
Wouldn’t a global flag make it harder to use the jsoo compiler as a library, which could be invoked many times with different targets? |
For example, an online ocaml repl, or a “godbolt explorer” output visualizer |
Sure, but we already have that issue with other flags, in particular, effects. |
1d48011
to
bb6474d
Compare
Done. |
@OlivierNicole, the CI is unhappy, and the diff has extra lines because of formatting. Can you run |
0a92618
to
5a9d1d2
Compare
There was a subtyping relation not being automatically used under 4.12. The CI should be happy now. |
2d6fbb3
to
a98d687
Compare
a98d687
to
abb8540
Compare
Co-authored-by: hhugo <[email protected]>
What's the semantics of nativeInt with the wasm backend? |
They’re on 32 bits. |
Then we should use | NativeInt of Int32.t |
Done. (Edit: and well spotted!) |
@OlivierNicole, @vouillon, I'm tempted to introduce a dedicated type & module for target int. What do you think ? |
Is the idea to have Sure, it sounds better, but IMO this looks like an improvement over the present PR that could be merged separately. |
That's a good idea, indeed. |
@OlivierNicole @vouillon I've rebased and adapted #1693. |
Thank you for the exceptional review work. |
@OlivierNicole, what's the next step for ocaml-wasm/wasm_of_ocaml#47 |
Regression from ocsigen#1655 . Not sure if `Config.target` should be better handled as a parameter?
I am in the process of merging the JSOO |
Regression from #1655 . Not sure if `Config.target` should be better handled as a parameter?
This is the wasm_of_ocaml-side adaptations after ocsigen/js_of_ocaml#1655.
Integrate "Target-specific code" (ocsigen/js_of_ocaml#1655)
Co-authored-by: Olivier Nicole <[email protected]> Co-authored-by: Jérôme Vouillon <[email protected]> Co-authored-by: Hugo Heuzard <[email protected]>
Note: This PR depends on #1649 and #1654 and is based on top of them.
The proposed feature is contained only in the last commit.edit: These PRs have been merged.This PR contains @vouillon's work to have two distinct targets: Javascript and WebAssembly.
Dune support for the new Wasm backend has been proposed at ocaml/dune#10695.
This is part of a series of PRs intending to reduce the diff between js_of_ocaml and wasm_of_ocaml (see ocaml-wasm/wasm_of_ocaml#47).