-
Notifications
You must be signed in to change notification settings - Fork 26
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
Feat/compiler #483
base: main
Are you sure you want to change the base?
Feat/compiler #483
Conversation
Have a nice holiday if it's a holiday) |
Small update - I was fed up working with official antlr4 typescript target. Migrated to antlr4ng |
Have you looked at Treesitter at all? (question, not a suggestion, but I hear great things about it and might be where I'd start if I was going from scratch today). |
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.
Looking great, these are mostly optional thoughts at this point, up to you to incorporate them now or hold off a bit.
) { | ||
state.files[name] = content; | ||
state.isCompiled = false; | ||
this.compile(state); | ||
}, |
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.
Ok. Please do your best in the meantime to keep this as tidy as you can.
@@ -11,6 +11,7 @@ export interface SubroutineInfo { | |||
type: SubroutineType; | |||
localVarsCount?: number; | |||
} | |||
|
|||
export type GlobalSymbolTable = Record<string, GenericSymbol>; |
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 see in several places this is used specifically as a map, rather than an ad-hoc object. Should this be Map<string, GenericSymbol>
? It'll enforce type checking the possibly null .get
response, but will also make it easier to check .size
instead of Object.keys(...)
(Edit: this is the current interface with the compile
, so that would also need to change. Agreed to do that leter.)
@@ -22,13 +22,14 @@ | |||
"@nand2tetris/projects": "^1.0.0", | |||
"@nand2tetris/runner": "^1.0.0", | |||
"@types/node": "^20.14.2", | |||
"antlr4": "^4.13.2", | |||
"antlr4ng": "^3.0.7", |
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.
Does this change need a change in the README?
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've already changed the README. Both of these libraries implement the same api/interfaces
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 library already has a ton of comments in typescript sources so I don't know if makes sense to add anything else in terms of documentation
files: Record<string, string>, | ||
cmd: Command, | ||
): Record<string, string | CompilationError> { | ||
if (files.type == "LexerOrParserError") { | ||
throw new Error("Expected tree but got a lexer or parser error"); | ||
} | ||
const result: Record<string, string | CompilationError> = {}; | ||
for (const name of Object.keys(files)) { | ||
result[name] = ""; | ||
} | ||
const trees: Record<string, ProgramContext> = {}; | ||
const errors: Record<string, CompilationError> = {}; |
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.
For most of these - are these Objects or Maps? (It's OK if they're records, but please leave a comment if that's a specific design choice, and why Record is preferred to Map)
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.
These are maps. I will change it
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.
@DavidSouther Why to choose Map in these cases?
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.
After doing a small research on this topic I don't see any substantial difference other than a little bit better api.
return errors; | ||
} | ||
const validateTree = treeOrErrors as ProgramContext; | ||
const vmWriter = new VMWriter( |
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.
Ahh, I added a comment earlier about Record vs Map, but VMWriter wants a Record. Hmm, maybe that should have been a Map too. Up to you to change to using Maps, or acknowledging the comment and moving on.
What do you hear specifically? It's a pure C library with language bindings. I didn't work with such combination. Probably it has it's own pain points. I don't know. If you would build this from scratch, based on my experience, the most go to options are
|
Testing is done from my subjective point of view. |
Improved Jack compiler. Underneath the covers it uses ANTLR
List of improvements:
Cons:
Don't get scared because it says 13k LOC. Most of it are tests and test files. The src changes is around 1k LOC. This doesn't include the generated files.
You can check it out on https://happytomatoe.github.io/web-ide/compiler
TODO: