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 warning for unused macro args #1591

Closed
wants to merge 1 commit into from

Conversation

Rangi42
Copy link
Contributor

@Rangi42 Rangi42 commented Jan 1, 2025

Fixes #1579

I'm a bit wary of how much new warning output this will create for various projects. But, even those that do -Weverything can always add -Wno-unused-macro-arg if they find the workarounds (e.g. assert isconst("use all args: \#")) to be awkward/annoying.

@Rangi42 Rangi42 added enhancement Typically new features; lesser priority than bugs rgbasm This affects RGBASM labels Jan 1, 2025
@Rangi42 Rangi42 added this to the 0.9.1 milestone Jan 1, 2025
@Rangi42 Rangi42 requested a review from ISSOtm January 1, 2025 21:34
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.

Hm, let's hope that this doesn't come back to bite us later. I don't see anything wrong, but for some reason I don't feel confident either.

@Rangi42
Copy link
Contributor Author

Rangi42 commented Jan 1, 2025

No hurry in merging it, then. I'll play around with it some more.

Edit: we should check the behavior of this case:

MACRO foo
  if \1
    \2
  else
    \3
  endc
ENDM

@aaaaaa123456789
Copy link
Member

I've seen macros with dummy arguments on purpose. I've written said macros.
I don't mind extra warnings (tools existing beats tools not existing), but this is the kind of thing you'd want off by default. Anyone using -Weverything is asking for ALL THE OUTPUT, so they deserve what they get. For everyone else, this would be a useful thing for projects that can use it, but it shouldn't be turned on by default.

@Rangi42
Copy link
Contributor Author

Rangi42 commented Jan 2, 2025

Yes, this is enabled only with -Wall.

@aaaaaa123456789
Copy link
Member

I'd make it -Wextra, but that's me.

@Rangi42
Copy link
Contributor Author

Rangi42 commented Jan 2, 2025

Any examples from gbdev chat/projects of mistakes that this would have caught? if not, we're both iffy enough about it that IMO it should probably be closed.

@ISSOtm
Copy link
Member

ISSOtm commented Jan 4, 2025

Then let's close it. This has received only criticism.

@ISSOtm ISSOtm closed this Jan 4, 2025
@Rangi42 Rangi42 removed this from the 0.9.1 milestone Jan 4, 2025
@Rangi42 Rangi42 deleted the unused-macro-arg branch January 4, 2025 08:41
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] Warn when a macro argument is not used
3 participants