-
Notifications
You must be signed in to change notification settings - Fork 23
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
Translation: Resolve conflicts in matching.ml
and translcore.ml
#179
Conversation
Reviewing |
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.
leaving an unresolved conversation for internal thread
eb67984
to
eba4129
Compare
…sclass`, `translobj`, `translprim`, `translattribute`, `switch`) (#182) * Get transclass to build * Get translobj to build * Get translmod to build * Get translprim to build * Get translattribute.ml to build * Get switch to build
#179 was merged into this. Only the first commit needs to be reviewed here. |
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.
Looked at the first commit (matching.ml and translcore.ml) only, looks good.
Thanks all, merging (not squashing as 1 of the 2 commits in this PR corresponds to a stacked PR). |
(Actually, looks like rebase and merge is disabled and it's not so bad to squash, so I'll do that.) |
Cherry-picks a commit from #171; once that's merged I'll drop the
lambda.ml
andlambda.mli
changes from here.Nature of the resolutions:
Immediate | Pointer
toPfield
in addition tomutable_flag
. We addedfield_read_semantics
internally (this is semantically equivalent tomutable_flag
, though the constructor names are different). The conflicts are easy to resolve: I just figure outimmediate_or_pointer
based on the upstream piece of the diff.ap_inlined
differently at one callsite (though I doubt it actually matters, as I explain in a comment).Testing
To see that the latter is ok, I recommend adding this module to the top of the file:
Then it should build.