-
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
Generalized program tree #1076
Generalized program tree #1076
Conversation
Thanks. Would you mind restricting this PR To just introducing an additional IR. I don't want to review too many moving pieces at once. |
Yeah, sure. Will do!
вс, 13 дек. 2020 г., 21:57 hhugo <notifications@github.com>:
… Thanks. Would you mind restricting this PR To just introducing an
additional IR. I don't want to review too many moving pieces at once.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#1076 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAZXBBBTN7B2EPGSIV5N6TSUUFBXANCNFSM4UZWP2RQ>
.
|
0069feb
to
a679834
Compare
I've trimmed this PR down to only introducing an additional IR. |
20d1fec
to
0d49ce5
Compare
compiler/lib/ir.mli
Outdated
| RawSubstitution of expression | ||
|
||
and expression = | ||
| ERaw of raw_segment list |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we leave ERaw
to a later PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've simplified ERaw
as much as possible, but I'm not sure how to avoid it when plugging the new IR. caml_js_expr
is currently parsing the JS code into the tree and embeds that directly. Converting JS tree to IR does not sound like a great idea.
I meant, restrict the PR to a new IR and plug it in - but exclude things that change the behavior |
2ac01a3
to
4fa830a
Compare
Gotcha, I've restored the PR to original state and tweaked it a bit to produce exactly the same output. Tweaks included using |
4fa830a
to
9219d32
Compare
| Continue_statement of Javascript.Label.t option | ||
| Break_statement of Javascript.Label.t option | ||
| Return_statement of expression option | ||
| Labelled_statement of Javascript.Label.t * (statement * Loc.t) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the IR contain primitives like this that aren't present in OCaml?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These control flow structures are well recognized and exist in some form in all high-level languages. What would you suggest for the IR? I didn't want to make non-cosmetic changes to generate.ml
, at least in this PR. These primities don't exist in OCaml, but control flow deconstruction produces them right now and they are not too much specific for JavaScript (in Golang backend, these got transformed to Go AST seamlessly).
Hey @hhugo, have you had a chance to look more at this? |
I'm writing this as an outsider but is this new IR type sufficiently general? I wouldn't expect constructs like Is it feasible to handle the JS features in |
As of
I'm not completely sure if these could be somehow moved away from I'm all for making the IR as general as possible, just wanted to take baby steps in the right direction :) |
Or what you suggest @mnxn is to emit some |
Exactly. |
Sounds good to me! Question to maintainers if this belongs to this PR, or better organized as subsequent one. |
I'm looking into moving JS specific primitives outside of Some are quite obvious like Ideally primitives should be handled at two levels - first one would be Probably passing a primitive from one level to another can be plugged in somewhere here: js_of_ocaml/compiler/lib/generate.ml Line 1194 in 65c1fc1
ECall for primitives that we do not know - we can emit some EPrim IR node which can then be processed later by the target language backend.
I have not yet fully understood what that |
9219d32
to
66396df
Compare
66396df
to
45fc55f
Compare
Is there still interest in such change ? Should it be rebased or closed ? |
Not from my end unfortunately. Let's close this probably. |
This PR is a part of effort to address #1075. Scope of this PR is limited to only introduce the new IR type. Most of the work presented in this PR is due to @jordwalke (see rehp, I only brushed it up it and prepared for upstreaming.