Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

New lexer 2½: store expansions from the inside out #829

Merged
merged 1 commit into from
Apr 16, 2021

Conversation

Rangi42
Copy link
Contributor

@Rangi42 Rangi42 commented Apr 16, 2021

This shortens the lexer by 100 lines and simplifies access to expansion contents, since it usually needs the deepest one, not the top-level one.

(This eliminates the lookupExpansion macro, since all three uses can now be an ordinary for (struct Expansion *exp = lexerState->expansions; exp; exp = exp->parent) loop.)

Fixes #813 and resolves a FIXME comment in shiftChar

Note that this forks off of PR #799; that one is simpler and can be reviewed+merged on its own.

@NieDzejkob I'd really appreciate if you run the fuzzer on this branch to make sure the previous issue is resolved and see if there are any segfaults left.

@Rangi42 Rangi42 added this to the v0.5.0 milestone Apr 16, 2021
@Rangi42 Rangi42 requested review from ISSOtm and meithecatte April 16, 2021 02:08
@Rangi42 Rangi42 added refactoring This PR is intended to clean up code more than change functionality rgbasm This affects RGBASM labels Apr 16, 2021
@Rangi42 Rangi42 force-pushed the lexer-3 branch 7 times, most recently from d282cfb to 82f89e7 Compare April 16, 2021 03:14
@ISSOtm
Copy link
Member

ISSOtm commented Apr 16, 2021

@Rangi42 There are still conflicts, and the PR still contains 7 commits; shouldn't the first 6 be dropped instead, since #799 has been merged?

This shortens the lexer by 100 lines and simplifies
access to expansion contents, since it usually needs the
deepest one, not the top-level one.

Fixes gbdev#813
@Rangi42
Copy link
Contributor Author

Rangi42 commented Apr 16, 2021

Yes, I had branched+PRed it separately because I wasn't expecting it to be reviewed so quickly. :D

It's rebased to master now, will see if the tests still pass.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
refactoring This PR is intended to clean up code more than change functionality rgbasm This affects RGBASM
Projects
None yet
Development

Successfully merging this pull request may close these issues.

peekInternal: Assertion `ofs < expansion->len' failed.
2 participants