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

Disable EQUS expansion for raw symbols (by parsing them as strings) #1648

Merged
merged 7 commits into from
Feb 15, 2025

Conversation

Rangi42
Copy link
Contributor

@Rangi42 Rangi42 commented Feb 5, 2025

Fixes #1605

The goal here is to make #raw string symbols get treated like string literals/expressions instead of getting EQUS-expanded. The long-term goal is to implement that in a way compatible with removing EQUS expansion for all symbols (#905).

This approach works by just having the lexer disable EQUS expansion for all #raw symbols, so they get lexed as ID (or LABEL). Then most of the work is done in the parser. There are three kinds of things: clearly numeric expressions (42, 2 + 3, etc), clearly string expressions ("lol", STRSUB("hello", 1, 4), etc), and identifiers, whose role depends on their context and the type of the symbol corresponding to that identifier. So now, in addition to the relocexpr_no_str distinction, it needs a string_literal distinction.

This PR needs further comparison with the alternative lexer-based approach implemented in #1649.

@Rangi42 Rangi42 changed the title Disable EQUS expansion for raw symbols ("the parser approach") Disable EQUS expansion for raw symbols (by parsing them as strings) Feb 5, 2025
@Rangi42 Rangi42 added enhancement Typically new features; lesser priority than bugs rgbasm This affects RGBASM labels Feb 5, 2025
@Rangi42 Rangi42 added this to the 0.9.2 milestone Feb 5, 2025
@Rangi42 Rangi42 force-pushed the no-raw-expansion branch 4 times, most recently from fec4d46 to e5ce516 Compare February 7, 2025 15:51
@Rangi42
Copy link
Contributor Author

Rangi42 commented Feb 7, 2025

What feels natural to me about this:

  • The lexer doesn't have to depend on the runtime definition of a symbol. If it's raw and not followed by a colon, it gets lexed as a SYMBOL, and it's the parser's job to check whether that symbol is defined as a string or not.

The downsides:

  • Every place where the parser can accept a numeric or string value has to do that checking. Currently that means db, dw, dl, print, and strfmt, as well as relocexprs in general.
  • More lines of code than the alternative.

@Rangi42 Rangi42 requested a review from ISSOtm February 7, 2025 16:13
@Rangi42 Rangi42 marked this pull request as draft February 8, 2025 22:16
ISSOtm
ISSOtm previously requested changes Feb 12, 2025
Copy link
Member

@ISSOtm ISSOtm left a comment

Choose a reason for hiding this comment

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

I think I like this PR better than the lexer approach, as it feels more coherent from a language PoV. (In fewer words: “less lexer hack = good”).

I do feel that, while this PR is less hacky, it's also more fragile, due to all the code repetition; however, as I suggested in one of my comments, I think this could be solved by doing a bit of refactoring.

I also did not duplicate my comment from #1649 on the man page, but since that change is identical, it applies here too.

@Rangi42 Rangi42 marked this pull request as ready for review February 12, 2025 20:03
@Rangi42 Rangi42 requested a review from ISSOtm February 14, 2025 20:02
@Rangi42 Rangi42 removed the request for review from ISSOtm February 15, 2025 09:44
@Rangi42
Copy link
Contributor Author

Rangi42 commented Feb 15, 2025

Thanks to ISSOtm for helping settle on and refactor this approach! <3

@Rangi42 Rangi42 merged commit b2e865e into gbdev:master Feb 15, 2025
27 checks passed
@Rangi42 Rangi42 deleted the no-raw-expansion branch February 15, 2025 09:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Typically new features; lesser priority than bugs rgbasm This affects RGBASM
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature request] Raw #symbols would not go through EQUS expansion
2 participants