-
Notifications
You must be signed in to change notification settings - Fork 602
perly - simplify non-terminal bare_statement_for #23873
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: blead
Are you sure you want to change the base?
Conversation
30c084c to
4d18cbb
Compare
7335e53 to
43552bd
Compare
forming sequence `KW_FOR remember`. Non-terminal `remember` is empty non-terminal with action starting new full lexical scope. Such empty non-terminals, when used in multiple branches, will generate shift/reduce conflict when not appearing after distinguishable sequence of terminals. Change formalizes `KW_FOR` as a keyword starting new lexical scope.
PERLY_PAREN_OPEN mexpr PERLY_PAREN_CLOSE
to visualize possible deduplication
Extract multiple ways how to declare cursor variables of for loop into dedicated non-terminal allowing to reduce number of branches in bare_statement_for.
43552bd to
7f8a73f
Compare
| PERLY_PAREN_OPEN | ||
| remember | ||
| PERLY_PAREN_OPEN |
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.
Commit [parser] bare_statement_for - move remember right after keyword forming sequence KW_FOR remember.
This commit message is still a bit too cryptic for me.
Non-terminal
rememberis empty non-terminal with action starting
new full lexical scope. Such empty non-terminals, when used in
multiple branches, will generate shift/reduce conflict when not
appearing after distinguishable sequence of terminals.
I simply can't understand most of that..
Reading the Such empty non-terminals, when used in multiple branches, will generate shift/reduce conflict makes me wonder if there is/was an issue with how it was written before and/or if there is any change to how the construct is now treated.
Change formalizes
KW_FORas a keyword starting new lexical scope.
The commit message should also what the consequence of this is. (Does it change anything? If yes then what? If no then explicitly mention it in the commit message).
Also missing a bit of the "why"; What was the real problem (if any) with the old code?
| %type <opval> bare_statement_when | ||
| %type <opval> bare_statement_while | ||
| %type <opval> bare_statement_yadayada | ||
| %type <opval> clause_mexpr |
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.
Commit [parser] clause_mexpr - deduplicate shared pattern into non-terminal PERLY_PAREN_OPEN mexpr PERLY_PAREN_CLOSE
For this commit I would use something like:
Add `clause_mexpr` to remove duplicated code
The pattern `PERLY_PAREN_OPEN mexpr `PERLY_PAREN_CLOSE` is repeated in several places
add `clause_mexpr` to avoid this repetition.
This change should not affect any of the behaviour.
In my opinion having: [parser] clause_mexpr and non-terminal and PERLY_PAREN_OPEN mexpr PERLY_PAREN_CLOSE in the commit title does not add any real value. (The title is too long and reading it in git log --oneline I would really need to think hard what it really means and if it might alter behavior or not)
perly.y
Outdated
| remember | ||
| KW_MY | ||
| my_scalar | ||
| my_scalar[cursor] |
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.
Commit [parser] bare_statement_for - rename variable(s) product to cursor to visualize possible deduplication
To me (but I'm unfamiliar with the parser) variable(s) product is meaningless/confusing.
I also find the word cursor confusing (but again could be because I'm unfamiliar with the code and/or non-native English). I don't know what to expect when I see the word cursor..
| %type <opval> bare_statement_when | ||
| %type <opval> bare_statement_while | ||
| %type <opval> bare_statement_yadayada | ||
| %type <opval> clause_for_cursor |
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.
Commit [parser] clause_for_cursor - encapsulate FOR loop cursor variables
This is already an improvement;
What I would still add in the commit message is what branches are now merged.
I.e. something like:
Merge the for-branches of:
- `for $scalar (...)`
- `for my $scalar (...)`
- `for my ($foo, $bar) (...)`
- `for \ $scalar (...)`
- `for my \ $scalar (..)`
into a single branch with the parsing of the variables handled by `clause_for_cusror`
By extracting various declarations of cursor variable(s) into dedicated non-terminal
for statement now contains only 3 branches:
for (;;)for ()for cursor ()with LALR(1) parser
cursorcannot be empty -> it will produce conflict withfor (;;)form.