Skip to content
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

internal: reorder ParsedNodeKind for range checks #907

Merged
merged 4 commits into from
Sep 21, 2023

Conversation

bung87
Copy link
Contributor

@bung87 bung87 commented Sep 16, 2023

Summary

Reorder ParsedNodeKind for range checks, and added named sets to
support querying parsed node kinds.

Details

Reorder ParsedNodeKind in order to support ranges over similar kinds.
This is considered experimental as the motivation for the reodering came
from usage in suggest (a consumer). The ordering should be determined
by the parser (the producer). Creating these seemingly convenient
consumer/read side ranges end up quietly coupling the producer and
consumer(s). Which results in refactorings to a small collection of
parser modules spilling out to changes in various consumer locations. In
this case these might coincide with the parser's needs, as well.

Additionally, added pnkDeclarativeDefs and pnkRoutineDefs similar to
PNode's kind, TNodeKind.

@saem saem added refactor Implementation refactor compiler/lex-parse Parsing and lexing subsystem of the compiler labels Sep 17, 2023
@saem saem added this to the Tooling milestone Sep 17, 2023
Copy link
Collaborator

@saem saem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm trying to understand the reasoning for the movement, or put another way, what are the group principles being applied to come up with the new ordering.

the type section I can sort of understand (guessing), but I'm struggling to come up with the reasons for the const def move, especially when taking ident def into account. The important part isn't whether identdef should move or not, but what are the rules/principles we should be following so we can make sure things are consistent.

compiler/ast/ast_parsed_types.nim Outdated Show resolved Hide resolved
compiler/ast/ast_parsed_types.nim Show resolved Hide resolved
@bung87 bung87 requested a review from saem September 19, 2023 16:55
Copy link
Collaborator

@saem saem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally speaking, I'm not a huge fan of reordering the enum based on the needs of a consumer, the order needs to be producer centric as the primary need is parsing.

Creating these seemingly convenient consumer/read side ranges end up quietly coupling the producer and consumer(s). Which results in refactorings to a a small collection of parser modules spilling out to changes in various consumer locations.

Consumers are better off using provided or created named sets for their querying need. I'm ok with this change going through because it doesn't trouble the parser yet, but this is something to be mindful of going forward.

@saem
Copy link
Collaborator

saem commented Sep 21, 2023

Updating the PR title and body.

If I have further questions, i'll ping @bung87 in chat.

@saem saem changed the title reorder ParsedNodeKind;add pnkDeclarativeDefs,pnkRoutineDefs like PNode internal: reorder ParsedNodeKind for range checks Sep 21, 2023
@saem
Copy link
Collaborator

saem commented Sep 21, 2023

/merge

@github-actions
Copy link

Merge requested by: @saem

Contents after the first section break of the PR description has been removed and preserved below:


Notes for Reviewers

part of #879

@chore-runner chore-runner bot added this pull request to the merge queue Sep 21, 2023
Merged via the queue into nim-works:devel with commit 37263e3 Sep 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler/lex-parse Parsing and lexing subsystem of the compiler refactor Implementation refactor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants