-
Notifications
You must be signed in to change notification settings - Fork 463
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
AST cleanup: use inline record for Pexp_fun. #7213
Conversation
b5a7309
to
f6dbcc8
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 improves readability so much! ❤️
CHANGELOG.md
Outdated
@@ -40,6 +40,7 @@ | |||
- AST cleanup: store arity in function type. https://github.com/rescript-lang/rescript/pull/7195 | |||
- AST cleanup: remove explicit uses of `function$` in preparation for removing the type entirely. https://github.com/rescript-lang/rescript/pull/7206 | |||
- AST cleanup: remove `function$` entirely. https://github.com/rescript-lang/rescript/pull/7208 | |||
- AST cleanup: use inline record for Pexp_fun. https://github.com/rescript-lang/rescript/pull/7213 |
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 should go to alpha.7.
compiler/ml/pprintast.ml
Outdated
@@ -543,7 +543,7 @@ and expression ctxt f x = | |||
| Pexp_let _ | Pexp_letmodule _ | Pexp_open _ | Pexp_letexception _ | |||
when ctxt.semi -> | |||
paren true (expression reset_ctxt) f x | |||
| Pexp_fun (l, e0, p, e, arity) -> | |||
| Pexp_fun {arg_label=l; default= e0; lhs= p; rhs= e; arity} -> |
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 not formatted with ocamlformat? As there are different spaces around =
.
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.
Good question.
Perhaps it gets stuck somewhere in the file.
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.
It was not formatted as there are some cppo ifdef's.
There are all these cppo configs:
(env
(dev
(env-vars
(CPPO_FLAGS -U=RELEASE)))
(release
(env-vars
(CPPO_FLAGS -D=RELEASE))
(ocamlopt_flags
(:standard -O3 -unbox-closures)))
(static
(env-vars
(CPPO_FLAGS -D=RELEASE))
(ocamlopt_flags
(:standard -O3 -unbox-closures)))
(browser
(env-vars
(CPPO_FLAGS -D=BROWSER))
(ocamlopt_flags
(:standard -O3 -unbox-closures))))
do we actually need them? All of them?
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.
Soo much better!
f6dbcc8
to
9829671
Compare
No description provided.