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

Allow {symbol} interpolation outside of strings #634

Merged
merged 1 commit into from
Dec 13, 2020

Conversation

Rangi42
Copy link
Contributor

@Rangi42 Rangi42 commented Dec 11, 2020

Fixes #629

Closes #631

@Rangi42 Rangi42 force-pushed the interpolation branch 3 times, most recently from 8541fe9 to b03f3b3 Compare December 11, 2020 21:42
@ISSOtm
Copy link
Member

ISSOtm commented Dec 11, 2020

Unfortunately, this implementation suffers from a bug that macro arguments used to have (#63).

issotm@sheik-kitty ~/rgbds% ./rgbasm - <<< 'S = 42                        [interpolation|✔] 
quote> PRINTV 0{d:S}0
quote> '
ERROR: <stdin>(2):
    syntax error
error: Assembly aborted (1 errors)!
$0

The proper fix for that issue was to expand arguments before everything else (hence peek itself performing the macro arg expansion).

@Rangi42 Rangi42 force-pushed the interpolation branch 2 times, most recently from 1c97438 to 4c88907 Compare December 12, 2020 02:31
@Rangi42
Copy link
Contributor Author

Rangi42 commented Dec 12, 2020

Now peek is solely responsible for doing symbol interpolation outside of strings. (readString has its own readInterpolation call because its peek calls need to have interpolation disabled.)

This fixes the above issue; interpolation will work inside of numeric literals.

@ISSOtm
Copy link
Member

ISSOtm commented Dec 12, 2020

Why do strings need interpolation disabled?

@Rangi42
Copy link
Contributor Author

Rangi42 commented Dec 12, 2020

Inside readString, peek needs to not do interpolation itself, since that can lead to invalid syntax. For example, in PRINTT "{__FILE__}\n", __FILE__ is "\"file-sym.asm\"", so without the lexerState->disableInterpolation = true, it would see PRINTT ""file-sym.asm"".

@ISSOtm
Copy link
Member

ISSOtm commented Dec 12, 2020

Alright, I'm not a fan of this special case, but it's necessary for compatibility, and the implementation is acceptable.

Checkpatch notes two style mismatches.

Finally, as this is a touchy part of the codebase, I'll page @AntonioND for a second pair of eyes; if he's satisfied with the change as well, he can merge this.

@ISSOtm ISSOtm requested a review from AntonioND December 12, 2020 17:45
@Rangi42
Copy link
Contributor Author

Rangi42 commented Dec 13, 2020

This, plus more printf-like format specifiers, will really simplify pokered's code.

TechnicalMachines:
n = 1
REPT NUM_TMS
	db TM{02d:n}_MOVE
n = n + 1
ENDR
;\1 source map
def_warps_to: MACRO
; output and purge each _WARP_TO_NUM_<N> warp_to, from N=1 to _NUM_WARPS
_WARP_TO_WIDTH = \1_WIDTH ; used in each _WARP_TO_NUM_<N>
n = 1
	REPT _NUM_WARPS
		_WARP_TO_NUM_{d:n}
		PURGE _WARP_TO_NUM_{d:n}
n = n + 1
	ENDR
ENDM

The "output and purge" comment there is basically self-explanatory.

@AntonioND
Copy link
Member

Ooops, I didn't see the review request. I've received so many notifications from this repo in the last few days that I've just started to get rid of most of them...

My problem with this PR is that it is just not clear what it does. There is no documentation and the only test is so long and complicated that is not useful as an example.

@ISSOtm
Copy link
Member

ISSOtm commented Dec 13, 2020

The lack of documentation is certainly a problem. I would define the content of this PR as "expanding the contents of symbols, like in strings, but in code".

@AntonioND If you're receiving too many notifications, you may want to tune your "Watch" settings?

@AntonioND
Copy link
Member

The lack of documentation is certainly a problem. I would define the content of this PR as "expanding the contents of symbols, like in strings, but in code".

Yeah, true.

@AntonioND If you're receiving too many notifications, you may want to tune your "Watch" settings?

Well, most of the time this isn't a problem because there isn't that much activity, and I like the instant notifications in that case. In situations like the current one, when there are so many changes, just ping me on Discord to make sure I see it.

@Rangi42
Copy link
Contributor Author

Rangi42 commented Dec 13, 2020

I've simplified the test case file and added it to the man documentation. It covers the possible uses of {interpolation}:

  • Applied to strings
  • Applied to numbers
  • Nested
  • Using format specifiers
  • Interpolating within ordinary code context
  • Interpolating within a numeric literal
  • Interpolating within a name to be defined
  • Interpolating within a name to be purged

Please let me know if the example should be edited further.

Copy link
Member

@AntonioND AntonioND left a comment

Choose a reason for hiding this comment

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

Yeah, that's better.

@ISSOtm
Copy link
Member

ISSOtm commented Dec 13, 2020

Alright, I'm OK with the new changes as well, so we'll (finally!) merge this.

@ISSOtm ISSOtm merged commit ce58f6d into gbdev:master Dec 13, 2020
@Rangi42 Rangi42 deleted the interpolation branch December 13, 2020 23:33
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.

[Feature request] An EVAL function to look up strings as symbols
4 participants