-
Notifications
You must be signed in to change notification settings - Fork 15
Support for labeled tuples #48
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
Conversation
|
@alanechang - you've been nominated to review this. See the note above about a suggested review path. |
alanechang
left a comment
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.
Reviewed this following the suggested path. Mainly check for test completeness and correctness. Also added a few notes on style. Did not find any big issues and agree that an error here will probably be easy to catch when deploying.
|
(This is ready to be merged, but I am going to hold off on clicking the button until the relevant compiler has been released so we don't accidentally ship it before then.) |
|
Nothing bad happens if ocamlformat gets ahead of the compiler, right? Not necessarily disagreeing with your plan to wait -- more checking my own understanding. |
Shipping a new ocamlformat ahead of the relevant feature has caused problems for apply style in the past, but I confess I do not recall the details. (I'll try to figure out what that issue was and write it down somewhere tomorrow) |
This is, unfortunately, a largeish PR. I think it's not worth reviewing line-by-line in detail, but suggest instead looking at whether the tests are sufficient and doing a quick style review on the rest. (If it's wrong enough to break styling of current features, we'll find that out quickly before we deploy, and if the styling of labeled tuples isn't perfect we can just improve it later).
A few things that make it tricky (but, which, again, I suggest not actually reviewing the implementation of in detail):
parser-standardis in a very bad state right now. As discussed offline, it's using a version of jane syntax that can't easily be upgraded, so I had to rewrite parts of the labeled tuples jane syntax modules to be compatible.parser-standardparser itself is out of date (and can't be fully updated now), and copying in the labeled tuples parsing from theflambda-backendparser resulted in new parser conflicts (this is why there are changes to make the parsing oflocal_in function types to be a little more like the current upstream version - this fixed the conflicts).(x:int * bool) -> unitneeds parens but the parameter ofint * x:bool -> unitdoesn't).let _ = ~x:x, ~y:y in ...is rewritten as a punlet _ = ~x, ~y in .... These two actually parse the same, so we must do this. But we can't rewritelet _ = ~x:(x [@attr]), ~y in ...because there'd be no place to put the attribute.flambda-backend's parser are just strings, but here they need to be augmented with locations to support accurate comment placement.