-
Notifications
You must be signed in to change notification settings - Fork 15
Erase layout annotations as a part of --erase-jane-syntax
#61
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
Signed-off-by: alanechang <alanechang@janestreet.com>
Signed-off-by: alanechang <alanechang@janestreet.com>
Signed-off-by: alanechang <alanechang@janestreet.com>
Signed-off-by: alanechang <alanechang@janestreet.com>
Signed-off-by: alanechang <alanechang@janestreet.com>
0ba87c2 to
f98013a
Compare
goldfirere
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.
This implementation looks sensible, but see a comment in the test -- I think we disagree about what the spec here is.
Signed-off-by: alanechang <alanechang@janestreet.com>
goldfirere
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.
Ah, yes, I forgot to review the second half!
Signed-off-by: alanechang <alanechang@janestreet.com>
Signed-off-by: alanechang <alanechang@janestreet.com>
Signed-off-by: alanechang <alanechang@janestreet.com>
This PR adds support for erasing layout annotations when the
--erase-jane-syntaxflag is provided.This includes parser standard changes:
jane.erasable.layoutsto have them dropped during ast normalization. Implementation here differs from the jane syntax encoding used by the flambda-backend parser. This is meant as a temporary solution before we can do straightforward parser imports to make them the same again. The alternative of porting this PR doesn't seem to be worth the effort.float#-->floatrewrites.And parser extended changes:
(... as _)is invalid syntax)This PR notably doesn't make any changes toSee update[@@immediate]and[@@immediate64]. Those attributes remain the same through this process.This PR should be merged after #60.Already mergedUpdate regarding
[@@immediate]and[@@immediate64]Last three commits added to this PR changes the treatment of these attributes.
Now ocamlformat rewrites both of these attributes into equivalent layout annotations by default. And when running with
--erase-jane-syntax, the program does the opposite and rewrites immediate/immediate64 layout annotation on type declarations back into attributes.Make sure this flambda-backend PR gets merged before this change to not break existing code.mergedMight be easier to review all the changes before this and then the immediate rewrite commits separately.