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

Jump to bottom

Attempt to warn when use of ternary op might fail due to side effects #241

Closed
andythenorth opened this issue Oct 10, 2021 · 1 comment · Fixed by #243
Closed

Attempt to warn when use of ternary op might fail due to side effects #241

andythenorth opened this issue Oct 10, 2021 · 1 comment · Fixed by #243

Comments

@andythenorth
Copy link
Contributor

andythenorth commented Oct 10, 2021 •
edited

Discovered via https://gist.github.com/andythenorth/64f3532ee4c148c5d7f9bf21ba36cda1#file-gistfile1-txt

FIRS code using procedures and storage access in a ternary op appeared to be incorrectly evaluating as true

e.g. foo() < bar() ? a : b was returning True for a case where value of foo() and bar() should have been 12 and 9, e.g. 12 < 9 ? a : b returned a.

glx diagnosed this as a case where temp register 0x100 (?) is re-used within the chain, which causes incorrect results.

The fix is to not use ternary op in these cases. glx has a commit which might function as a warning in these cases.
@glx22
Copy link
Contributor

glx22 commented Oct 10, 2021

No the issue is not the reuse of temp register 0x100, but the way ternary are compiled.

For guard ? expr_true : expr_false, first guard is evaluated, then stored as guard, and it's opposite stored as !guard. Then some magic happens for the result, it's translated as !guard * expr_false + guard * expr_true, meaning both expressions are evaluated in any case.

If any of both expression writes to a register, you have an unexpected side effect.

@andythenorth andythenorth changed the title Attempt to warn when use of ternary op might fail due to over-writing temp register (?) Attempt to warn when use of ternary op might fail due to side effects Oct 10, 2021
@glx22 glx22 mentioned this issue Oct 10, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Add: Warn when ternary operator results are not read only glx22/nml
2 participants
@andythenorth @glx22