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

Logical operators || and && are short-circuiting #665

Closed
wants to merge 1 commit into from

Conversation

Rangi42
Copy link
Contributor

@Rangi42 Rangi42 commented Dec 26, 2020

true || X short-circuits to 1 without evaluating X
false && X short-circuits to 0 without evaluating X

Fixes #619

@Rangi42
Copy link
Contributor Author

Rangi42 commented Dec 26, 2020

Note: This comment is obviated by #685, which makes println "{X}" and mymacro "{X}" both expand {X} as within a string literal.

The new test case does not use a print macro to combine the printv and printt "\n" because that affects how interpolation interacts with short-circuiting.

In printv 1 == 2 && 0 < STRLEN("{undef_s}"), the printv 1 == 2 && is lexed, the parser enters the relocexpr T_OP_LOGICOR mid-action rule and starts short-circuiting, and the subsequent 0 < STRLEN("{undef_s}") is lazily evaluated.

In print 1 == 2 && 0 < STRLEN("{undef_s}"), the print is lexed, the parser enters the T_ID mid-action rule for macro and does lexer_SetMode(LEXER_RAW), and then the lexer in raw mode reads the rest of the line and since it's doing both interpolation and macro arg expansion without actually tokenizing anything, it interpolates {undef_s} and complains about the undefined value.

This is arguably how it should work, or at least, changing the behavior would affect this case:

print2: MACRO
n = 20
	printv \1
	printt "\n"
ENDM

n = 10
	print {n}

This currently prints 10, not 20. Edit: which is definitely a good thing, since that's how you can pass {_NARG} to another macro.

@Rangi42
Copy link
Contributor Author

Rangi42 commented Dec 26, 2020

Another subtlety of short-circuiting: both macro arguments and interpolation outside strings can expand to just about anything, so I don't think those should necessarily get short-circuited in the first place.

For example, db 2+2==4 || \1. Here the \1 looks like it ought to be a self-contained valid operand for the ||, and in real use cases it probably is. But it could also have been false \n printt "Hi!" in which case the user might expect "Hi!" to be printed regardless of the db above it. Or the line could have been db (1==1 || \3 without a closing parenthesis, where the user is for some reason expected to provide that as part of the macro argument; if short-circuiting did not expand the \3 (on the grounds that there might be a third macro arg) then it would just cause a syntax error.

@Rangi42 Rangi42 force-pushed the short-circuit branch 3 times, most recently from ca6978c to 5f01e1e Compare December 30, 2020 16:04
@Rangi42 Rangi42 changed the title Logical operators || and && are short-circuiting.asm Logical operators || and && are short-circuiting Dec 31, 2020
@Rangi42 Rangi42 force-pushed the short-circuit branch 9 times, most recently from e2985c1 to 3ddd38a Compare January 6, 2021 17:59
@Rangi42
Copy link
Contributor Author

Rangi42 commented Jan 6, 2021

This might be worth adding to 0.4.3 even if a later release will add lazily-evaluated expressions and revamp how short-circuiting works, because as-is this can already improve performance by not evaluating right-hand sides.

@Rangi42 Rangi42 force-pushed the short-circuit branch 4 times, most recently from 388e182 to 524330f Compare January 15, 2021 15:58
@Rangi42 Rangi42 force-pushed the short-circuit branch 3 times, most recently from 5ce3c7a to 90507ff Compare January 19, 2021 14:16
`true || X` short-circuits to 1 without evaluating X
`false && X` short-circuits to 0 without evaluating X

Fixes gbdev#619
@Rangi42
Copy link
Contributor Author

Rangi42 commented Jan 21, 2021

The short-circuiting semantics would be most useful for user-defined functions, which don't exist yet. User-defined functions would depend on true RPN lazy evaluation anyway, and @ISSOtm discussed why that's a better long-term solution in #663.

@Rangi42 Rangi42 closed this Jan 21, 2021
@Rangi42 Rangi42 deleted the short-circuit branch January 21, 2021 14:44
@Rangi42 Rangi42 mentioned this pull request Jul 27, 2021
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.

Make && and || short-circuiting
1 participant