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

Implement INCLUDE_ONCE directive #1481

Merged
merged 4 commits into from
Sep 8, 2024
Merged

Implement INCLUDE_ONCE directive #1481

merged 4 commits into from
Sep 8, 2024

Conversation

sukus21
Copy link
Contributor

@sukus21 sukus21 commented Aug 21, 2024

Fixes #1478.

This behaves like regular include the first time a file is included, except the name of the file is added to a list. The next time using INCLUDE_ONCE on that file, the file will not be included. INCLUDE_ONCE does not take symlinks into account. The string that is registered, is the one entered into the directive.

This is effectively an include guard, not unlike C/C++ #PRAGMA ONCE.

@Rangi42
Copy link
Contributor

Rangi42 commented Aug 21, 2024

Thank you for this contribution! However, it's somewhat premature; a feature-request issue is already open, and the blocker is "getting community confirmation that this is worth adding", not "being able to implement it technically".

@Rangi42 Rangi42 added enhancement Typically new features; lesser priority than bugs rgbasm This affects RGBASM labels Aug 21, 2024
@Rangi42 Rangi42 marked this pull request as draft August 21, 2024 20:10
@Rangi42 Rangi42 changed the title Implement 'INCLUDE_ONCE' directive Implement INCLUDE_ONCE directive Aug 21, 2024
@ISSOtm ISSOtm added this to the v0.9.0 milestone Sep 4, 2024
src/asm/fstack.cpp Outdated Show resolved Hide resolved
src/asm/fstack.cpp Outdated Show resolved Hide resolved
src/asm/fstack.cpp Outdated Show resolved Hide resolved
src/asm/parser.y Outdated Show resolved Hide resolved
@sukus21 sukus21 requested a review from Rangi42 September 4, 2024 20:38
@sukus21 sukus21 marked this pull request as ready for review September 5, 2024 12:35
@Rangi42
Copy link
Contributor

Rangi42 commented Sep 5, 2024

I pushed to this myself to update the man page wording/formatting a bit. Edit: And to fix the failing test.

Copy link
Contributor

@Rangi42 Rangi42 left a comment

Choose a reason for hiding this comment

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

Thank you for your work on this! Looks good to me, if we choose to add this feature. Pending @ISSOtm's approval as well.

@Rangi42 Rangi42 requested a review from ISSOtm September 5, 2024 14:50
ISSOtm
ISSOtm previously requested changes Sep 7, 2024
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.

What about a.asm includes d/b.inc then b.inc, with -I d?

@ISSOtm
Copy link
Member

ISSOtm commented Sep 7, 2024

Furthermore, what about -P, symlinks, and other platform-specific goodness? Path manipulation introduces more platform-specific behaviour, and I don't like going further down that avenue.

@Rangi42
Copy link
Contributor

Rangi42 commented Sep 8, 2024

-P calls the same fstk_RunInclude function that INCLUDE does, as it always has. -I is handled via the trivially cross-platform fstk_FindFile "path manipulation" function, as it always was. We both know that nobody is going to symlink bar.inc to foo.inc and rely on INCLUDE_ONCE treating them both the same, but I went ahead and handled that over-the-edge case anyway. :P

@ISSOtm ISSOtm closed this Sep 8, 2024
@Rangi42 Rangi42 reopened this Sep 8, 2024
@Rangi42 Rangi42 merged commit 5f07095 into gbdev:master Sep 8, 2024
44 checks passed
Rangi42 added a commit to Rangi42/rgbds that referenced this pull request Sep 8, 2024
@Rangi42 Rangi42 removed this from the v0.9.0 milestone Sep 8, 2024
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: #pragma once or include_once equivalent
3 participants