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

parser: add error for ambiguous operator precedence #18341

Closed
wants to merge 1 commit into from

Conversation

nektro
Copy link
Contributor

@nektro nektro commented Dec 22, 2023

special-cased groupings of +- (plus the wrapping and saturating variants) and */ to absolve the most common and obvious occurrence.
all other non-parenthesized groupings of same-precedence operators will trip this.

Fixes #1902
Fixes #114

@nektro nektro force-pushed the patch-6 branch 2 times, most recently from 0c51479 to cfe9dad Compare December 22, 2023 07:33
@nektro nektro force-pushed the patch-6 branch 2 times, most recently from 6f78207 to 00e1c4b Compare December 23, 2023 02:03
Comment on lines +1702 to +1703
if (prev_op == .plus and tok_tag == .minus) break :blk;
if (prev_op == .minus and tok_tag == .plus) break :blk;
if (prev_op == .plus_percent and tok_tag == .minus_percent) break :blk;
if (prev_op == .minus_percent and tok_tag == .plus_percent) break :blk;
if (prev_op == .plus_pipe and tok_tag == .minus_pipe) break :blk;
if (prev_op == .minus_pipe and tok_tag == .plus_pipe) break :blk;

if (prev_op == .asterisk and tok_tag == .slash) break :blk;
if (prev_op == .slash and tok_tag == .asterisk) break :blk;
Copy link
Member

Choose a reason for hiding this comment

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

IMO it'd be better to allow mixing all operators with the same precedence (except maybe || and ** which could also have their precedences raised) but I'm fine with this too.

Copy link
Member

@andrewrk andrewrk left a comment

Choose a reason for hiding this comment

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

I like all the changes except for these two.

However, let's start with @Vexu's suggestion and then re-evaluate.

@nektro nektro force-pushed the patch-6 branch 2 times, most recently from 653f53d to 70247a5 Compare January 20, 2024 23:15
@nektro
Copy link
Contributor Author

nektro commented Feb 20, 2024

pinging since its been a month

@andrewrk
Copy link
Member

This bitrotted before I had a chance to look at it again. I'm sorry for that. Please open a fresh one if you want to roll the dice again.

@andrewrk andrewrk closed this Aug 24, 2024
@nektro nektro deleted the patch-6 branch August 24, 2024 05:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants