Support leading logical operators #3337
Merged
+229
−7
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kddnewton
force-pushed
the
leading-logical
branch
2 times, most recently
from
263df49 to
877f497
Compare
kddnewton
force-pushed
the
leading-logical
branch
3 times, most recently
from
75236a8 to
a10344c
Compare
Morriar
reviewed
Jul 16, 2025
Comment on lines
+10856
to
+10849
| (peek_at(parser, following) == '.') || | ||
| (peek_at(parser, following) == '&' && peek_at(parser, following + 1) == '.') |
Collaborator
There was a problem hiding this comment.
Would it be better to use store peek_at(parser, following) into an intermediate variable?
Comment on lines
+10871
to
+10866
| (peek_at(parser, following) == '&' && peek_at(parser, following + 1) == '&') || | ||
| (peek_at(parser, following) == '|' && peek_at(parser, following + 1) == '|') || | ||
| (peek_at(parser, following) == 'a' && peek_at(parser, following + 1) == 'n' && peek_at(parser, following + 2) == 'd' && !char_is_identifier(parser, following + 3, parser->end - (following + 3))) || | ||
| (peek_at(parser, following) == 'o' && peek_at(parser, following + 1) == 'r' && !char_is_identifier(parser, following + 2, parser->end - (following + 2))) |
Collaborator
There was a problem hiding this comment.
Same question here about all the peek_at(parser, following)
Earlopain
added a commit
to Earlopain/prism
that referenced
this pull request
Jul 21, 2025
This makes it hard to do version checks against this value. The current version checks work because there are so few possible values at the moment. As an example, ruby#3337 introduces new syntax for ruby 3.5 and uses `PM_OPTIONS_VERSION_LATEST` as its version guard. Because what is considered the latest changes every year, it must later be changed to `parser->version == parser->version == PM_OPTIONS_VERSION_CRUBY_3_5 || parser->version == PM_OPTIONS_VERSION_LATEST`, with one extra version each year. With this change, the PR can instead write `parser->version >= PM_OPTIONS_VERSION_CRUBY_3_5` which is self-explanatory and works for future versions.
Earlopain
added a commit
to Earlopain/prism
that referenced
this pull request
Jul 21, 2025
This makes it hard to do version checks against this value. The current version checks work because there are so few possible values at the moment. As an example, ruby#3337 introduces new syntax for ruby 3.5 and uses `PM_OPTIONS_VERSION_LATEST` as its version guard. Because what is considered the latest changes every year, it must later be changed to `parser->version == parser->version == PM_OPTIONS_VERSION_CRUBY_3_5 || parser->version == PM_OPTIONS_VERSION_LATEST`, with one extra version each year. With this change, the PR can instead write `parser->version >= PM_OPTIONS_VERSION_CRUBY_3_5` which is self-explanatory and works for future versions.
Earlopain
reviewed
Jul 21, 2025
src/prism.c
Outdated
| (peek_at(parser, following) == '.') || | ||
| (peek_at(parser, following) == '&' && peek_at(parser, following + 1) == '.') | ||
| )) | ||
| (parser->version == PM_OPTIONS_VERSION_LATEST) && |
Collaborator
There was a problem hiding this comment.
I found this version check a bit strange since LATEST is a moving target and openend #3605 to talk about it.
Collaborator
There was a problem hiding this comment.
PR above was merged, this should now be this
Suggested change
| (parser->version == PM_OPTIONS_VERSION_LATEST) && | |
| (parser->version >= PM_OPTIONS_VERSION_CRUBY_3_5) && |
Earlopain
reviewed
Aug 6, 2025
src/prism.c
Outdated
| (peek_at(parser, following) == '.') || | ||
| (peek_at(parser, following) == '&' && peek_at(parser, following + 1) == '.') | ||
| )) | ||
| (parser->version == PM_OPTIONS_VERSION_LATEST) && |
Collaborator
There was a problem hiding this comment.
PR above was merged, this should now be this
Suggested change
| (parser->version == PM_OPTIONS_VERSION_LATEST) && | |
| (parser->version >= PM_OPTIONS_VERSION_CRUBY_3_5) && |
src/prism.c
Outdated
| LEX(PM_TOKEN_AMPERSAND_DOT); | ||
| } | ||
|
|
||
| if (parser->version == PM_OPTIONS_VERSION_LATEST) { |
Collaborator
There was a problem hiding this comment.
Suggested change
| if (parser->version == PM_OPTIONS_VERSION_LATEST) { | |
| if (parser->version >= PM_OPTIONS_VERSION_CRUBY_3_5) { |
Member
|
FYI Matz approved this feature a week ago in https://bugs.ruby-lang.org/issues/20925#note-11 |
nobu
force-pushed
the
leading-logical
branch
from
a10344c to
0e67140
Compare
kddnewton
force-pushed
the
leading-logical
branch
from
0e67140 to
3f58fa7
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Per https://bugs.ruby-lang.org/issues/20925