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

Support -P/--preinclude to pre-INCLUDE a file #1043

Merged
merged 2 commits into from
Sep 24, 2022

Conversation

Rangi42
Copy link
Contributor

@Rangi42 Rangi42 commented Sep 1, 2022

Fixes #1041

@Rangi42 Rangi42 added enhancement Typically new features; lesser priority than bugs rgbasm This affects RGBASM labels Sep 1, 2022
src/asm/fstack.c Outdated Show resolved Hide resolved
src/asm/main.c Outdated Show resolved Hide resolved
@Rangi42 Rangi42 requested a review from ISSOtm September 1, 2022 23:57
test/asm/test.sh Outdated Show resolved Hide resolved
@Rangi42
Copy link
Contributor Author

Rangi42 commented Sep 2, 2022

I would use -I constants.asm instead of INCLUDing that all the time when/if this is merged.

@Rangi42 Rangi42 force-pushed the includefile branch 2 times, most recently from 1d48ef4 to 48dd13b Compare September 2, 2022 04:01
@ISSOtm
Copy link
Member

ISSOtm commented Sep 2, 2022

The main problem I have with this, is users trying to get into a codebase, not figuring out where all of these symbols are defined. Again, what's wrong with INCLUDE "constants.asm"?

@eievui5
Copy link
Contributor

eievui5 commented Sep 2, 2022

Constants which should be in every single file should be there by default without the programmer having to remember them. Users not knowing where constants are defined is already a problem with -D or a master file in a unity build or exported constants (something you recommend). This just makes the experience a little less repetitive for the file or two which is needed everywhere. If it’s something someone doesn’t like because it might be confusing, they can simply not use the feature; the same applies to GCC’s -i.

@Rangi42
Copy link
Contributor Author

Rangi42 commented Sep 2, 2022

There are pretty many possible sources of implicit behavior beyond what's in an .asm file that users have to learn about: -D values, the linkerscript, Makefile rules that either build all .asm files or only an explicit list, even the -h and -L transformations and the -p byte if they're wondering about the particular bytes in the ROM.

src/asm/fstack.c Outdated Show resolved Hide resolved
src/asm/fstack.c Outdated Show resolved Hide resolved
@Rangi42 Rangi42 force-pushed the includefile branch 2 times, most recently from 8b7694e to dd26761 Compare September 5, 2022 19:37
@Rangi42 Rangi42 changed the title Support -I/--includefile to pre-INCLUDE a file Support -f/--includefile to pre-INCLUDE a file Sep 5, 2022
@Rangi42 Rangi42 force-pushed the includefile branch 2 times, most recently from 7838aed to db5688e Compare September 5, 2022 19:42
src/asm/fstack.c Outdated Show resolved Hide resolved
src/asm/fstack.c Outdated Show resolved Hide resolved
@Rangi42
Copy link
Contributor Author

Rangi42 commented Sep 8, 2022

Following discussion with @ISSOtm , call this -P/--preinclude, only allow one such file, and support -i include paths but not -MG.

@Rangi42 Rangi42 changed the title Support -f/--includefile to pre-INCLUDE a file Support -P/--preinclude to pre-INCLUDE a file Sep 10, 2022
@ISSOtm
Copy link
Member

ISSOtm commented Sep 24, 2022

Made some changes to the docs locally to ensure they render correctly.

src/asm/fstack.c Show resolved Hide resolved
src/asm/fstack.c Show resolved Hide resolved
@Rangi42 Rangi42 merged commit b8385a5 into gbdev:master Sep 24, 2022
@Rangi42 Rangi42 deleted the includefile branch September 24, 2022 16:38
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] Prologues (from the command line)
5 participants