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

Correct operator precedence #1571

Open
wants to merge 1 commit into
base: extui-align
Choose a base branch
from

Conversation

pstef
Copy link

@pstef pstef commented Oct 23, 2024

This change is untested but it should work :)

In the previous version, != took precedence over ^ so that (((*A)>>11)&0x1) != 0 was evaluated first and then xored with the left hand side.

Here I remove all the unnecessary parens except the ones suggested by GCC -Wparentheses. And add the parens that are actually needed in order to evaluate !=0 last.

In the previous version, != took precedence over ^ so that
(((*A)>>11)&0x1) != 0 was evaluated first and then xored with the
left hand side.

Here I remove all the unnecessary parens except the ones suggested
by GCC -Wparentheses. And add the parens that are actually needed
in order to evaluate !=0 last.
@FCare
Copy link
Owner

FCare commented Oct 24, 2024

I do not know exactly which flag or version of gcc has this issue, i did not dig in the debugging of gcc. Kronos is using gcc since the beginning and inherited lot of those unary operator code from yabause.

We found during fixing that unary usage requires a lot of care with parenthesis. I am not open to remove any parenthesis here. Just add a new one since it may introduce regressions more than fixes.

I agree it is not really readable but it works. If clang is complaining for the equality, just add one parenthesis, not more.

Thanks

@pstef
Copy link
Author

pstef commented Oct 24, 2024

I've found the flag and corrected the commit, GCC should not be complaining. Would it be ok for you as it is now?

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