Skip to content
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

Merged
merged 2 commits into from
Dec 24, 2024
Merged

Conversation

cristianoc
Copy link
Collaborator

No description provided.

@cristianoc cristianoc force-pushed the pexp_fun_inline_record branch from b5a7309 to f6dbcc8 Compare December 23, 2024 07:27
@cristianoc cristianoc requested review from cknitt and zth December 23, 2024 07:27
Copy link
Member

@cknitt cknitt left a 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
Copy link
Member

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.

@@ -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} ->
Copy link
Member

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 =.

Copy link
Collaborator Author

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.

Copy link
Collaborator Author

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?

Copy link
Collaborator

@zth zth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Soo much better!

@cristianoc cristianoc force-pushed the pexp_fun_inline_record branch from f6dbcc8 to 9829671 Compare December 24, 2024 07:56
@cristianoc cristianoc enabled auto-merge (rebase) December 24, 2024 08:03
@cristianoc cristianoc merged commit a99e84d into master Dec 24, 2024
19 checks passed
@cristianoc cristianoc deleted the pexp_fun_inline_record branch January 2, 2025 09:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants