Make comparison operators non-associative #24155
Merged
Conversation
This file contains 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
Signed-off-by: Danila Fedorin <[email protected]>
Signed-off-by: Danila Fedorin <[email protected]>
mppf
reviewed
Jan 5, 2024
| @@ -456,7 +456,7 @@ | |||
| %left TOR | |||
| %left TAND | |||
| %left TEQUAL TNOTEQUAL | |||
| %left TLESSEQUAL TGREATEREQUAL TLESS TGREATER | |||
| %nonassoc TLESSEQUAL TGREATEREQUAL TLESS TGREATER | |||
There was a problem hiding this comment.
What about != and ==? I would think that they are in the same category.
Signed-off-by: Danila Fedorin <[email protected]>
Signed-off-by: Danila Fedorin <[email protected]>
Signed-off-by: Danila Fedorin <[email protected]>
Signed-off-by: Danila Fedorin <[email protected]>
Signed-off-by: Danila Fedorin <[email protected]>
Signed-off-by: Danila Fedorin <[email protected]>
I really would not like to parse these myself, which I interpret as additional rationale for the change. Signed-off-by: Danila Fedorin <[email protected]>
Signed-off-by: Danila Fedorin <[email protected]>
Signed-off-by: Danila Fedorin <[email protected]>
This was referenced Mar 1, 2024
2 tasks
DanilaFe
added a commit
that referenced
this pull request
Mar 8, 2024
This PR builds on #24155. Closes #24559. In #24155, we made `a < b < c` a syntax error, because it leads to confusing results: a boolean is implicitly converted to a numeric value, and compared against another number. I went about disallowing such syntax by simply removing the associativity from `<`, `>`, etc. However, this meant that Bison simply reported "syntax error" when it encountered such an expression, which leaves much to be desired. This PR improves on that situation by making adjustments to the parser. Specifically, it enables the parser to note whether or not parentheses were used for an expression (including an operator comparison). This is done using the additional location maps added by @dlongnecke-cray in #23548. The parser is thus allowed to accept `a < b < c`, and subsequent checks are executed that detect `a < b < c` (as opposed to `(a < b) < c`, which is valid). These checks use the Dyno error system to print a nice error message. <img width="1153" alt="Screen Shot 2024-03-07 at 3 45 34 PM" src="https://github.com/chapel-lang/chapel/assets/4361282/02ac537a-1b51-4134-b800-1c6986ed9060"> Reviewed by @dlongnecke-cray -- thanks! ## Testing - [x] paratest - [x] dyno tests
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.
This is a potential solution to #24152. In particular, we accept the following program:
But this relies on the fact that
boolcan be interpreted numerically, and used in a comparison. It seems like the syntax0 <= n <= 2should not be allowed given a binary operation<=, because it's not associative. Our parser marks it%left, which seems wrong; this PR adjusts the comparison operators to be%noassoc.Reviewed by @mppf and agreed upon synchronously with the rest of the team.
Testing