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 defining local labels for another scope #1159

Merged
merged 2 commits into from
Aug 20, 2023

Conversation

pinobatch
Copy link
Member

fix #1157 for the following source code

section "hSAVE_locals",HRAM
func3.hSpam: ds 1  ; no longer produces an error
;.hEggs: ds 1      ; uncomment this to see the new error

section "demo",ROM0
func3:
  ldh a, [.hSpam]
  ret

Remove two errors:

  • Not currently in the scope of 'func3'
  • Local label 'func3.hSpam' in main scope

Add one error:

  • Relative local label '.hSpam' in main scope

Add a switch to restore previous behavior in include/asm/symbol.h

include/asm/symbol.h Outdated Show resolved Hide resolved
test/asm/sym-scope.asm Outdated Show resolved Hide resolved
@Rangi42 Rangi42 added enhancement Typically new features; lesser priority than bugs rgbasm This affects RGBASM labels Aug 17, 2023
ISSOtm
ISSOtm previously requested changes Aug 17, 2023
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.

The change seems good, but I agree with Rangi that the #define should be removed—I'm not sure there'd be value in letting this be toggled, especially at compile time?

src/asm/symbol.c Outdated Show resolved Hide resolved
test/asm/sym-scope.err Outdated Show resolved Hide resolved
@pinobatch pinobatch requested a review from Rangi42 August 19, 2023 12:31
@Rangi42
Copy link
Contributor

Rangi42 commented Aug 19, 2023

Looks good! Thanks for bearing with my comments. Just needs CI to pass.

@pinobatch
Copy link
Member Author

Thank you. Every job except checkpatch is passing. Does checkpatch compare only the states of the tree before and after the combined effect of the patches, or does it compare each patch? If the latter, how would I go about rewriting history to get the job to pass?

@Rangi42
Copy link
Contributor

Rangi42 commented Aug 19, 2023

how would I go about rewriting history to get the job to pass?

In your local repo, rebase this branch to master and squash all the commits, then force-push to remote.

fix gbdev#1157 for the following source code
```
section "hSAVE_locals",HRAM
func3.hSpam: ds 1  ; no longer produces an error
;.hEggs: ds 1      ; uncomment this to see the new error

section "demo",ROM0
func3:
  ldh a, [.hSpam]
  ret
```

Remove two errors:
- `Not currently in the scope of 'func3'`
- `Local label 'func3.hSpam' in main scope`

Add one error:
- `Relative local label '.hSpam' in main scope`

Add a switch to restore previous behavior in `include/asm/symbol.h`
```c
```

update tests of local label definition failure

now that local label definition is more lenient, fewer tests
should fail

begin removing issue number from test

Co-authored-by: Rangi <35663410+Rangi42@users.noreply.github.com>

treat ALLOW_LOCAL_LABEL_ANYWHERE as always on

- don't gate the new lenient behavior behind a build-time condition
- change "Relative local label" to "Unqualified local label"
  in an error message

realign tests of local labels with wrong parent

- repurpose test/asm/sym-scope from a test of producing correct
  errors to a successful test
- remove test/asm/local-wrong-parent because it is now a redundant
  successful test

based on suggestions by @Rangi42
@Rangi42 Rangi42 dismissed ISSOtm’s stale review August 19, 2023 16:19

Addressed review comments

Rangi42 suggested through Discord to bundle the zlib13
update (gbdev#1163) with this
@ISSOtm ISSOtm merged commit e1f0a13 into gbdev:master Aug 20, 2023
24 checks passed
@pinobatch pinobatch deleted the rm-parent-scope-requirement branch August 20, 2023 00:41
pinobatch added a commit to pinobatch/libbet that referenced this pull request Aug 25, 2023
- add missing "Closet" map name
- stpcpy as an example of forward-declaring a member of a local scope
- explain that forthcoming RGBDS 0.6.2 (or a master build allowing
  forward-declaring a member of a local scope) is required
  <gbdev/rgbds#1159>
- explain how to get a master build on Windows until 0.6.2 is ready
  <gbdev/rgbds-www#49>
- update URL of RGBDS
- Catskull is out of business
  <https://catskullelectronics.com/pages/the-end-of-catskull-electronics>
pinobatch added a commit to pinobatch/240p-test-mini that referenced this pull request Aug 25, 2023
- stpcpy and apustpcpy as examples of forward-declaring a member of a local scope
- explain that forthcoming RGBDS 0.6.2 (or a master build allowing
  forward-declaring a member of a local scope) is required
  <gbdev/rgbds#1159>
- explain how to get a master build on Windows until 0.6.2 is ready
  <gbdev/rgbds-www#49>
- update URL of RGBDS
- Catskull is out of business
  <https://catskullelectronics.com/pages/the-end-of-catskull-electronics>
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.

Define local labels outside the associated global label
4 participants