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

Simplify parsing and formatting of indexop-access expressions #2150

Merged
merged 1 commit into from
Sep 5, 2022

Conversation

gpetiot
Copy link
Collaborator

@gpetiot gpetiot commented Sep 1, 2022

Extracted from ocamlformat-ng's concrete AST in our long-running effort to make the AST closer to the original source (removing sugaring/desugaring).

The more noticeable change for the users is that the indexop-access having a RHS will break a sequence, which is more readable). That's the only diff visible when checking test_branch.sh on the preset profiles.

@gpetiot gpetiot force-pushed the exp-access branch 2 times, most recently from fcc9d0f to d44ee00 Compare September 2, 2022 17:55
@gpetiot gpetiot requested a review from Julow September 2, 2022 18:00
@gpetiot gpetiot marked this pull request as ready for review September 2, 2022 18:00
Copy link
Collaborator

@Julow Julow left a comment

Choose a reason for hiding this comment

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

Looks good and I like the formatting changes.

However, the patches on top of ocaml's parser are increasing in size faster than I expected. Updating to new versions of ocaml is harder after every such changes.

@gpetiot
Copy link
Collaborator Author

gpetiot commented Sep 5, 2022

However, the patches on top of ocaml's parser are increasing in size faster than I expected. Updating to new versions of ocaml is harder after every such changes.

Yes indeed, by the time the migration to a concrete syntax tree will be complete I don't think vendor/diff-parsers-std-ext.patch will be useful anymore. But hopefully the new parser will be much easier to read (no sugaring/normalization) and parsing rules would match the CST nodes almost on 1:1 ratio.

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