-
Notifications
You must be signed in to change notification settings - Fork 47
[v2] Added a new V2 AST module, with basic AST constructs #1493
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
base: main
Are you sure you want to change the base?
Conversation
|
cf16ecc to
1fc9485
Compare
crates/solidity-v2/outputs/cargo/cst/src/structured_cst/nodes.generated.rs
Outdated
Show resolved
Hide resolved
|
|
||
| pub fn new_yul_continue_keyword<'arena>( | ||
| _arena: &'arena Bump, | ||
|
|
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.
nit: WDYT of using YulUncheckedKeyword::new(...) instead?
impl YulUncheckedKeyword {
pub fn new(...) {
...
}
}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 think that'd be a better option, but it'd require some changes to the current representation, since sequence types are represented as a struct that is wrapped around an Rc<>, using a type alias. If we go down the line of impl new methods, it would require creating a full new type, like shown below:
pub struct AbicoderPragma {
pub inner: Rc<AbicoderPragmaStruct>,
}
impl AbicoderPragma {
pub fn new(...) ...
}
#[derive(Debug)]
pub struct AbicoderPragmaStruct {
pub abicoder_keyword: AbicoderKeyword,
pub version: AbicoderVersion,
}I think is too soon to know what the best representation will be, and this should eventually be replaced by @ggiraldez work (or a modification of that).
However, if this is something you think is worth to make the effort I'm happy to do it.
What do you think if, for now, I add a TODO comment with this expected change/improvement?
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.
As a note, there's one reason I can think on why this may not be a good idea down the road: some of these CST nodes should be translated to transparent nodes in the AST in the future.
For example, on the work @ggiraldez is doing for the ir2, a NamedImport is not present as a node, being just syntactic sugar for a PathImport. I expect the V2 parser to generate directly a PathImport, but ideally that logic shouldn't be part of the parser, so the parser would just call a function pub fn new_named_import(...) -> NamedImport, with NamedImport being just a type alias for PathImport.
It's not impossible to have both things, but in the future we may want to have the creation of AST nodes (all of these new_... methods) follow the CST nodes (ie what the parser sees), and have them completely separated from the AST structure (what the user sees).
This PR adds a new AST for V2 slang, it's still under active development so it will change.
There's a few things that will most likely change soon:
;)bumpalo, this should probably be abstracted away, and each constructor take a more complex type having an arena, the source, a string interner, etcOverhauling #1492