-
-
Notifications
You must be signed in to change notification settings - Fork 113
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
feat(compiler): Allow function re-exports to use regular call instruction #1176
Conversation
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.
Looks good (I think); just a couple of questions.
{ | ||
val_repr: ReprFunction(args, rets, _), | ||
val_fullpath: Path.PIdent(id), | ||
} as vd, | ||
) => { | ||
switch (Ident_tbl.find_opt(func_map, id)) { | ||
| Some({name: Some(name)}) => | ||
| Some(internal_name) => | ||
let external_name = Ident.name(vid); |
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.
What's the significance of changing to vid
instead of id
here (and line 1293)?
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.
This way is "guaranteed" to be correct, since it's the name that's included in the module signature rather than the name that's a part of the full path (which is something internal). 1293 was really just a bug 😶
I guess it wasn't really a bug since it doesn't affect anything, but id
could potentially be incorrect.
/* Exports are already reversed, so keeping the first of any name is the correct behavior. */ | ||
List.filter( | ||
({ex_name}) => | ||
if (Hashtbl.mem(exported, Ident.name(ex_name))) { | ||
fun |
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.
Is this just removing duplicates (this question probably belongs on the other 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.
Yeah, that's it. The work it does is a bit different now since the names are already supplied.
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 read the code through, and LGTM but I don't understand some of the deeper concepts yet, so take with a pinch of salt !
7447bb8
to
19e7a76
Compare
20c9221
to
6159ee4
Compare
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.
This is soooooo awesome! I looked at a couple snapshots and they look 🔥
I only had a couple comments.
6159ee4
to
7dfc921
Compare
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.
Approved! Just need CI to turn green
No description provided.