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

[Feature request] Warn when a macro argument is not used #1579

Closed
ISSOtm opened this issue Dec 18, 2024 · 5 comments
Closed

[Feature request] Warn when a macro argument is not used #1579

ISSOtm opened this issue Dec 18, 2024 · 5 comments
Assignees
Labels
enhancement Typically new features; lesser priority than bugs rgbasm This affects RGBASM

Comments

@ISSOtm
Copy link
Member

ISSOtm commented Dec 18, 2024

Macros will, logically, error out when they are passed too few arguments. But, by nature, extraneous arguments are simply ignored. This can be intentional, but I expect that this would be rare; so, I'm suggesting -Wunused-macro-arg enabled by -Wall.

In theory, we could also add some syntax for macros to specify how many arguments they want to be given; but I expect that adoption would be low, and thus I'm not sure it would be worth the implementation effort.

@ISSOtm ISSOtm added enhancement Typically new features; lesser priority than bugs rgbasm This affects RGBASM labels Dec 18, 2024
@Rangi42
Copy link
Contributor

Rangi42 commented Dec 18, 2024

There are definitely some macros which just take a "flag" argument whose actual value doesn't matter (often it's 1, could be TRUE if the codebase uses such a constant, but 0 or foobar or whatever would also work) and just influences behavior of a _NARG check. So if we have a warning like this, there should be a convenient way to disable the warning, i.e. one that could be done in the definition of the macro, not at every usage site.

Syntax "to specify how many arguments they want to be given" sounds like it would develop into #912.

@Rangi42
Copy link
Contributor

Rangi42 commented Dec 30, 2024

So, in this hypothetical:

macro foo
  pusho Wno-unused-macro-arg
    db \1, \3
  popo
endm
  foo 1, 2, 3

The popo would undo the Wno-unused-macro-arg before reaching the endm, but the unused argument check would have to occur at the endm. So this would still warn about \2 being unused.

On the other hand, users could just put a ; unused: \2 comment, if they want the warning enabled in general but not for this particular macro (the equivalent of (void)unused_arg; in C). Instances like this are rare enough that I think that would be acceptable.

(You could also do ; \# if you're not sure which args might be unused. Or /* \# */ if some of them might have newlines.)

@Rangi42 Rangi42 added this to the 0.9.1 milestone Dec 30, 2024
@Rangi42 Rangi42 self-assigned this Dec 30, 2024
@Rangi42
Copy link
Contributor

Rangi42 commented Jan 1, 2025

Oops, we don't expand things in comments, so ; \# or /* \# */ wouldn't work. (And on reflection, that's a good thing, since if an argument contained a newline or */ then ending a comment on expansion would be really annoying.)

This also does not work:

if 0
  println "\#"
endc

(In hindsight that also makes sense. If you did if _NARG < 2 ... else ... endc, you wouldn't want \3 to expand in the else block. And we don't allow trickery like an arg that expands to a control-flow-changing endc anyway.)

Things that do work:

redef _discard equs "\#"
assert isconst("\#")

Or, of course, local opt Wno-unused-macro-arg or global -Wno-unused-macro-arg.

@mid-kid
Copy link
Contributor

mid-kid commented Jan 1, 2025

Would shift be considered using an argument? Because if so, I figure that would be enough.

I'm also not entirely sure who the warning is for. Is it for the user of the macro, who might've specified too many arguments? Or, is it for the implementor of the macro, who might've forgotten to do something?
RGBDS has no real macro specification, they're all positional, and the error for specifying too few only happens when it's referenced.

In my opinion, given this constraint, I see no real circumstance in which shift 3 would mean the implementor has forgotten to use the first three arguments, as they're clearly being acknowledged (a macro invocation with less than 3 arguments would error out).

@Rangi42
Copy link
Contributor

Rangi42 commented Jan 2, 2025

Good point about shift; the current PR does not count that as a "use". Although I think most cases of false-positive unused-arg warnings don't even involve shift (note the update PR test cases, and the new complaints about building pokecrystal).

@ISSOtm ISSOtm closed this as not planned Won't fix, can't repro, duplicate, stale Jan 4, 2025
@Rangi42 Rangi42 removed this from the 0.9.1 milestone Jan 4, 2025
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 a pull request may close this issue.

3 participants