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

Replace use of binary 'or' with logical 'or' #2907

Merged
merged 1 commit into from
Jan 18, 2024

Conversation

DanilaFe
Copy link
Contributor

A recent Chapel PR (chapel-lang/chapel#24155) disallowed the use of expressions like a < b < c because of its confusing meaning. Specifically, a < b < c is not equivalent to a < b && b < c. Instead, the above expression is equivalent to (a < b) < c, which is likely not what the author intended. On main, Chapel requires explicit parentheses: (a < b) < c or a < (b < c).

This led to us uncovering some bugs that have to do with the use of binary OR | instead of the logical or ||. Notably, the the precedence for these operators is as follows: | > > > ||. Thus, a < b | c parses as a < (b | c), whereas a < b || c parses as (a < b) || c. This is significant in conditionals, such as those affected by the PR:

if xiBin < 0 | yiBin < 0 | (xiBin > (numXBins-1)) | (yiBin > (numYBins-1)) { /* ... */ }
//         ^^^^^^^^^ probably not what you want!
//                   Equivalent to:
if xiBin < (0 | yiBin) < (0 | (xiBin > (numXBins-1)) | (yiBin > (numYBins-1))) { /* ... */ }

As you can see, this was previously being interpreted as the ternary .. < .. < .. expression. This is both likely incorrect logically (I doubt the intention is to compare xiBin against yiBin or'ed with zero) and, on the main branch of the Chapel compiler, incorrect syntactically.

This PR fixes the syntax error caused by this issue, and updates the uses of the bitiwse | operator to be uses of the logical ||.

Copy link
Contributor

@bmcdonald3 bmcdonald3 left a comment

Choose a reason for hiding this comment

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

Looks good, thanks Daniel!

@bmcdonald3 bmcdonald3 added this pull request to the merge queue Jan 18, 2024
Merged via the queue into Bears-R-Us:master with commit 730e5d2 Jan 18, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants