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

fix(precedence): right-associative operators #549

Merged
merged 1 commit into from
May 22, 2020

Conversation

czosel
Copy link
Collaborator

@czosel czosel commented May 10, 2020

This fixes a regression introduced in #485

Fixes #548

@czosel czosel requested a review from ichiriac May 10, 2020 19:58
Copy link
Member

@ichiriac ichiriac left a comment

Choose a reason for hiding this comment

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

meaning that this.isRightAssociative(result.type) is considering that result.type === result.right.type is not right associative

I'm just wondering if the fix is meant only on this if branch, or should the isRightAssociative implementation be rewrote ?

Could you please check if it's the only case, or the bug may be more generic ?

@czosel
Copy link
Collaborator Author

czosel commented May 11, 2020

Hi @ichiriac,
thanks for the review! isRightAssociative is only called from the one if branch that was changed. I also thought about changing the implementation of isRightAssociative first, but the function only does one thing: It takes an operator, and returns true if the operator is right-associtive. In order to tell if the "swap" should be performed in resolvePrecedence, the right-associativity should only be considered if the two operators have the same precedence level. This is equivalent to the two operators being different, because ** and ??, the only two right-associative operators PHP knows, have different precedence.

The bug only affected ** and ??, I added a test for the latter.

@czosel czosel force-pushed the fix-right-associative-precedence branch from d0e5cd3 to 418d9ab Compare May 11, 2020 20:05
@czosel
Copy link
Collaborator Author

czosel commented May 20, 2020

@ichiriac friendly ping 🙃

@ichiriac
Copy link
Member

sorry, totally forgot this issue 😃

@ichiriac ichiriac merged commit 695c9b1 into master May 22, 2020
@ichiriac
Copy link
Member

The PR is merged, I'll try to fix #511 and release a patch, this week or next one

@czosel
Copy link
Collaborator Author

czosel commented May 22, 2020

No worries, thanks for merging! 🎉

@czosel czosel deleted the fix-right-associative-precedence branch May 22, 2020 15:12
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.

Precedence of exponentiation operator
2 participants