Streamline append VM implementation and remove enums #1643
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Historically, many of these enums used `const enum`s, which at least had the benefit of being compiled away at build time. We've now moved to mostly normal `enum`s, which are both less efficient than normal constants, and a relic of an earlier TypeScript era before literal types. This PR moves all enums to normal constants, and creates types for the constant values when the code needs them.
| @@ -7,7 +7,8 @@ export { | |||
| s, | |||
| unicode, | |||
| } from './lib/builder/builder'; | |||
| export { Builder, type BuilderStatement } from './lib/builder/builder-interface'; | |||
| export { type BuilderStatement } from './lib/builder/builder-interface'; | |||
| export * from './lib/builder/constants'; | |||
There was a problem hiding this comment.
can we export explicitly, instead of export *?
From a "knowing what is here" perspective, * feels like an unneeded indirection, and risks leaking things we don't actually want to expose as public API.
There was a problem hiding this comment.
there is a lot in here: https://github.com/glimmerjs/glimmer-vm/pull/1643/files#diff-f881a95cdbb07c9ee63e2f6fca320f7d00323b27434dbee753f4cffb38d30a85R6
does it all need to be public API?
There was a problem hiding this comment.
It was all public API but sucked into the Builder enum. And the codebase already does a lot of export *.
There was a problem hiding this comment.
is this file generated?
NullVoxPopuli
approved these changes
Oct 30, 2024
|
Ope -- this is a regression |
NullVoxPopuli
requested changes
Oct 30, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR begins the process of incrementally implementing many of the improvements in the error recovery branch.
I'm going to start with the easy stuff and work my way up to more complicated parts of the implementation, and in that spirit, this PR has two changes:
debugfield pattern. This simplifies the implementation and also makes it possible to consolidateInternalAppendVMinto the normalVM. The benefit of the "opcode-only" subset of the VM was waning anyway, and most of the remaining distinction had to do with private implementation details that are no longer present in theVMinterface because they're now implemented using private fields.const enumandenumwith normal constants and literal types. There was a good reason for us to useconst enumwhen we originally made the decision. But the dynamics have changed. Between (a) the fact that we can't reliably thatconst enumwill be compiled away anymore, and (b) the fact that TS now has literal types, using normal constants and literal types is a better choice.