Initial generation of IR from nu AST. #40
Conversation
|
This is the same as #30 (sorry for the duplicate PR), with initial comments applied. See math snapshots tests, as those are the ones that don't get any errors. I am playing with integrating it with the actual nushell in https://github.com/doriath/nushell/tree/new-parser (it is not submittable and is extremely hacky, but allows me to test if a simple IR actually can be executed) |
src/ir_generator.rs
Outdated
| r | ||
| } | ||
|
|
||
| fn span_to_string(&mut self, node_id: NodeId) -> Option<String> { |
There was a problem hiding this comment.
I can this use std::str::from_utf8() instead and return &str to avoid the String allocation?
Also, I'd move these two span_to_xxx() functions to the Compiler, and perhaps rename then to node_as_string/i64(), or similar.
There was a problem hiding this comment.
Sounds great, moved to Compiler and renamed.
src/ir_generator.rs
Outdated
| let mut spans = vec![]; | ||
| let mut ast = vec![]; | ||
| for _ in 0..(self.instructions.len()) { | ||
| spans.push(Span { start: 0, end: 0 }); |
There was a problem hiding this comment.
Is there a reason to separate generate() and block() into two functions? I believe you could create the IrBlock at the beginning and then while generating the nodes, it could push the instructions to it with a proper span on the fly.
Better than using a Span is to modify IrBlock to use NodeId, but this can be done later and in stages (e.g., first use both Span and NodeId until we migrate fully, then deletee Span), not in the scope of this PR.
The AST field can be None since we don't need the old Nushell AST for anything. Once we use NodeId instead of Span, as outlined above, then the NodeIds form the sub-AST corresponding to the IrBlock, I think that should suffice.
There was a problem hiding this comment.
I believe generate() and block() are separated because of errors and to follow other classes (like typechecker). I could aldo just change generate() to return Result<IrBlock, Vec<SourceError>>. WDYT?
Applied the suggestion to create IrBlock in constructor, and then modify it as part of the execution.
|
Left some comments, but overall looks like a good start! |
|
Thank you for the review! |
|
OK, seems better. One more thing, you can pass NodeId as a parameter to |
The initial implementation only handles simple math expressions, the focus is primarily on introducing new structs, agreeing on naming and adding the output to insta tests.
|
Updated to properly populate spans. |
|
Awesome, thanks! |
3 participants
The initial implementation only handles simple math expressions, the focus is primarily on introducing new structs, agreeing on naming and adding the output to insta tests.