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.
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
decouple parser from PNode #425
decouple parser from PNode #425
Changes from all commits
6103e24
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Yes, or at least one will be a super set of the other. Ultimately separate enums are likely best.
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 found it too!
OK, so this dumb thing is used to tell the semantic analysis layer about a statement list (possibly also block) that are being passed to a macro, template, whatever. During semantic analysis we collapse/flatten nested statement lists into one level, but we also do statement list unwrapping, so if there is only one node in a statement list we unwrap it except if there is a defer (I think this might be a bug/design flaw).
That first flattening is guarded by this dumb flag. I personally think we should always flatten, no matter what, but never unwrap in semStmtList (it's the last/second last proc in semstmts, I rewrote it recently so it should be readable). Without that guard a macro can receive arbitrary chunks of AST not always wrapped in a statement list and it makes the really annoying/painful to write.
I do believe between fixing the unpacking logic and the AST the parser generates this shouldn't be a problem. My guess at the most correct fix is sematic analysis layer should:
Then this dumb guard should not be necessary. Separating the AST used by sem, parsing, and backend means we can change add and index assign behavior and it only impacts sem so we can do destination based unwrapping rather than have it live incorrectly in semStmtList. 🎉
Sorry that was a rather lengthy explanation.
This comment was marked as resolved.
Sorry, something went wrong.
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 don't think so, but maybe I'm missing some key point? Whatchya thinking?
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.
We have node kinds for everything and only float literals. It is a naming question, and it is a set of node kinds, not a set of literals. Also
nkLiterals
is inconsistent with Atomic kinds in themacros.nim
, but that's another question. I thinknkLiteralKinds
,nkFloatKinds
or evennkFloatLiteralKinds
make more sense (the last one is the most sensible IMO, but a bit longer)TBH I always found this enum set definitions a bit weirdly structured and lacking in some places, but it is hard to see the underlying structure immediately from the code, it must be an incremental 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.
nimskull/compiler/ast/ast_query.nim
Line 687 in 837238f
[]
named indexing operator), but for now that's just a new idea I got while elaborating on the "literals/kinds" distinction aboveThere 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
nkFloatLiterals
vsnkFloatKinds
, the former is literals only and the latter might include more, but I can't think of a single thing not covered by the former. The is a difference in meaning but I'm really hard pressed to think of a case where they would not be equivalent. I would check the sites and consolidate personally.That's exactly the types of insights that form by looking at this stuff.
I agree with the sentiment, the naming and categorization needs to make sense based on purpose and not solely because a new programmer's first instincts are to lump them together.
So to recap:
nkLiterals
) or lots of literal handling is weird about floats vs other numbers (nkFloatLiterals
)Doesn't have to be those precise names but I think we're on the same page. For actually changing sets carefully looking through their usage is required because there are unfortunate subtleties.