-
Notifications
You must be signed in to change notification settings - Fork 448
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
Remove backend for lazy #6960
base: master
Are you sure you want to change the base?
Remove backend for lazy #6960
Conversation
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 code all dead? So pure refactoring?
Would be good to clarify in the PR description.
@@ -191,7 +191,7 @@ let rec classify_expression : Typedtree.expression -> sd = | |||
| Texp_ident _ | Texp_for _ | Texp_constant _ | Texp_new _ | Texp_instvar _ | |||
| Texp_tuple _ | Texp_array _ | Texp_construct _ | Texp_variant _ | |||
| Texp_record _ | Texp_setfield _ | Texp_while _ | Texp_setinstvar _ | |||
| Texp_pack _ | Texp_object _ | Texp_function _ | Texp_lazy _ |
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.
You can't remove an item or the entire runtime representation is reindexed and the editor extension will crash.
Instead you can give it a unit payload and add a comment that it is unused.
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.
I am wondering if we should rename the unused constructors to something like Texp_unused_lazy
.
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.
sure
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.
You can't remove an item or the entire runtime representation is reindexed and the editor extension will crash.
Instead you can give it a unit payload and add a comment that it is unused.
Could you explain how removing something from runtime related to the editor extension? I asked the same question in another PR, feel free to answer in a single place.
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.
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.
You can't remove an item or the entire runtime representation is reindexed and the editor extension will crash.
Instead you can give it a unit payload and add a comment that it is unused.Could you explain how removing something from runtime related to the editor extension? I asked the same question in another PR, feel free to answer in a single place.
The editor extension and the compiler operates on the same generated compiler artifacts. If we change the representation in these artifacts (which happens if we change the types enough) then things stop working.
In the future we'll have tools bound to a specific ReScript version so this isn't a problem. But right now that's not how it works, so we need to take that into account.
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.
The editor extension and the compiler operates on the same generated compiler artifacts.
The ones we have in printsmth
files? So we can change the payload, but the variant should stay, so the print logic stays the same, is it correct?
| Texp_lazy (e) ->
line i ppf "Texp_lazy";
expression i ppf e;
Is it safe to add new items to the variant?
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.
To the payload, or add a new variant case at the end?
ParseTree is still there to be removed