Unused variable/parameter cleanup #1321

Thar0 · 2022-07-09T17:28:57Z

Applies the UNUSED attribute to unused variables, enables checking -Wunused-variable warnings now that they're all suppressed.
Applies the UNUSED attribute to unused parameters in boot and code, overlays have many action functions where one or more parameter is unused so I've left those alone for now. Since not all -Wunused-parameter warnings are suppressed, I've left -Wno-unused-parameter in CHECK_WARNINGS.
Changes the names of variables starting with pad that are used.
Collapses adjacent padding variables of the same type into arrays where possible. We already do this for the most part however some were missed.
Makes padding variable names more consistent, for functions with one pad it is just named pad and for functions with multiple they are named pad1, pad2, etc. Also renamed some padding variables with the same name in different scopes.

…for all stack padding variables

Dragorn421

Do we want to label every "unused" thing the same way?

(looking at this like a "real" codebase you'd want to work on)
For example, in ascending order of "legitimate to keep but unused":

UNUSED s32 pad;
- There is no reason to keep this
Sched_Init(..., UNUSED UNK_TYPE arg3
- It is unclear if that argument is useful / what it's for
SkelCurve_Draw(UNUSED Actor* actor
- This argument is unused but you can imagine it having a purpose
is_proutSyncPrintf(UNUSED void* arg
- That argument is required to match the callback prototype, and this particular callback implementation simply has no use for it

I think (4) clearly deserves UNUSED since there's no way around it: it cannot not have that argument.
(3) to me deserves it too, it seems fair to pass the actor for drawing something, an implementation pass may eventually make use of it and at that point it would be a pain to go back and add the argument to every call.
(2) and (1) I think would be straight up removed in a clean-up pass, they aren't justified to exist, so I wouldn't think they deserve UNUSED

To be clear with (2) and (1), labeling them UNUSED feels like solving a warning in:

s32 i;
s32 j; // warning: j is unused

for (i=0;...

by silencing the warning:

s32 i;
UNUSED s32 j;

for (i=0;...

instead of just removing the unused thing:

s32 i;

for (i=0;...

I know we can't just remove the things but it doesn't make sense to silence the warnings that would definitely have been there in the original code too. We just can't fix everything unfortunately I guess.

EllipticEllipsis · 2022-07-12T19:30:45Z

Plus a lot of these pads are not actually pads, especially in actors, they're pending GameState recasts. Overall I'd say this is another thing that it's too early to do, since it doesn't actually do anything for us to turn off this warning.

Thar0 · 2022-07-29T16:33:07Z

(2) and (1) I think would be straight up removed in a clean-up pass, they aren't justified to exist, so I wouldn't think they deserve UNUSED

I agree that they would be removed, but since we can't do that I'd rather mark them with UNUSED, since we can do that at no detriment, and leave the removal step to others. Adding UNUSED doesn't inhibit their removal, nor is it particularly misleading as of course they are unused. If anything it makes it easier to remove/replace especially since we had a few variables named "pad.." that were actually used, so a simple pad regex is insufficient. Now with UNUSED a regex to remove them all is much simpler.

Plus a lot of these pads are not actually pads

That can be said for all pads assuming they were once meaningful variables. As before, adding UNUSED doesn't inhibit future changes and again I argue it makes them easier to automatically replace in the future if we wanted. (I'm not up to date on the latest discussion on this but I recall some are hesitant on whether we even should implement the gamestate casts)

fig02 · 2022-07-29T16:43:19Z

fwiw I agree with tharo and think this is fine to do now. marking something as unused isnt going to cement it as unused forever

Roman971 · 2022-07-29T20:57:52Z

I don't have a clear opinion on this topic yet but I'll try to give my current thoughts. Basically right now I feel like we shouldn't bother at all with including UNUSED macros for parameters, and probably not for pads either.

For unused parameters I have actually found that warnings are often more painful than useful even in a real codebase, particularly in languages where the compiler can't figure out whether the unused argument is required for some reason (eg. to match a common function interface like action functions). They can even make things worse if you follow them strictly, since it's often good to leave unused parameters to show that some kind of data or state is available in that function and could potentially be used.

For pads I think it's sort of pointless to have the UNUSED macro since they should already be named in a way that makes it clear they're not used. It does allow us to enable the unused variable warnings, but it's not like we gain much by having that warning enabled in the decomp repo itself, aside from ensuring pads aren't incorrectly named but that shouldn't really happen anymore with decompilation being done. So overall I don't know if it's worth bothering with it.

The only real case where I agree this macro should be used is for variables that are properly named and set but not actually used, because that's a situation where the variable serves no purpose and should definitely be removed in a normal situation.

Either way I think we should consider that a bunch of variables and/or parameters are going to be used in some versions and not others, so I don't know how viable it will be to have these UNUSED macros in the future. For instance are we going to ifdef versions just to have the macro or not on some parameters? 😰

Dragorn421 · 2022-07-29T23:08:39Z

So with the 1/2/3/4 cases I highlighted above,

I would want UNUSED on 3, 4 only
Tharo and Fig want UNUSED on all 1,2,3,4
Roman wants UNUSED on none? (?)

Nice to see we agree on this 😎

Thar0 · 2022-07-29T23:22:22Z

For unused parameters I have actually found that warnings are often more painful than useful even in a real codebase [...]

Part of the rationale for why __attribute__((unused)) exists is exactly for the situation you describe where some state is available but not (possibly as of yet) used. It's not like it exists for codebases like this with "weird" extra constraints (for us, matching).

I think unused-parameter is worth accommodating for in the decomp repo, even if you wouldn't use it personally (and I'm certain you wouldn't be alone), I'd rather we get as close to default warnings as is reasonable and allow modders to make the choice on what they want to do with respect to warnings. Choosing to disable it is much easier than choosing to enable it, as the latter will then require the modder to do a lot of work to fix it or be swamped with many warnings from vanilla code which they wouldn't even touch otherwise, leading to horrible merge conflicts with upstream.

For pads I think it's sort of pointless to have the UNUSED macro [...]

I really don't follow this viewpoint much at all. I think it's very odd to not mark the pads as unused, they are currently nothing but unused variables from our perspective and it only makes it easier to find/replace them.
You had this to say about set-but-unused variables:

because that's a situation where the variable serves no purpose and should definitely be removed in a normal situation

I think this extends just as much to totally unused variables as well. Totally unused variables definitely tick both these boxes.

it's not like we gain much by having that warning enabled in the decomp repo itself

It gets us closer to default warnings which I believe is more beneficial for the sake of modding and making it more like a "real" codebase. I'd rather not leave something out that we can viably accommodate for and expect a modding repo to have to fill in for us, making it increasingly difficult for modding repos to merge with upstream.

Either way I think we should consider that a bunch of variables and/or parameters are going to be used in some versions and not others [...]

I can see how it can get dicey with other versions, I don't have a good grasp on how much that will affect this admittedly, but I don't think it will change enough to render this totally unviable.

Dragorn421 · 2022-07-29T23:36:10Z

So we just assumed different goals with this I guess, afaict:

you are looking at this with the focus of "make warnings useful out of the box when modding from master",
I'm looking at it like "proper UNUSED usage in a real codebase",
and Roman is looking at it like "real programmers don't need warnings" ? 😎

Dragorn421 · 2022-10-07T21:46:37Z

My opinion on this hasn't changed, however there may be a middle-ground with tagging the unused-warning-raising stuff differently depending on its category 1/2/3/4 (see my comment above #1321 (review) )

Like, 1 and 2 could be USELESS instead of UNUSED (#define USELESS UNUSED) (or PAD_UNUSED, ...)
or/and we add that famous inline-nonmatching macro (#define IFOK(e) e if matching, otherwise #define IFOK(e)) and IFOK(s32 pad)

Thar0 · 2022-10-22T02:00:41Z

I don't mind the inline nonmatching macro, although if we were to use a macro we could use a dedicated macro for stack padding?
Maybe something like
#define STACK_PAD(type, n) { type pad[n]; } ?

I'm not so sold on the case for example (2) though. I don't think it's different enough to (3) to warrant doing anything different with it, UNUSED is perfectly accurate even if we can't discern possible usage at this time.

Dragorn421 · 2022-10-22T23:22:38Z

I can concede on using UNUSED for (2) as well I guess, it's not like there are a whole lot of these anyway so they aren't really in the way compared to stack padding temps

#define STACK_PAD(type, n) { type pad[n]; } exactly doesn't work because {} int temp; isn't C89 but why not idk

Dragorn421

Good job
I approve of the goal of the PR now (haven't looked at everything yet)

Makefile

include/functions.h

Co-authored-by: Dragorn421 <Dragorn421@users.noreply.github.com>

Dragorn421 · 2022-10-30T17:07:38Z

src/code/z_bgcheck.c

@@ -2438,7 +2437,7 @@ s32 BgCheck_AnyLineTest3(CollisionContext* colCtx, Vec3f* posA, Vec3f* posB, Vec
 * ignores `actor` dyna poly
 * returns true if any poly intersects the sphere, else false
 * `outPoly` returns the pointer of the first poly found that intersects
- * `outBgId` returns the bgId of the entity that owns `outPoly`
+ * @bug `outBgId` always returns BGCHECK_SCENE due to a bug in `BgCheck_SphVsFirstDynaPoly`


nice catch lol

Dragorn421 · 2022-10-30T18:19:51Z

src/overlays/actors/ovl_En_Zl3/z_en_zl3.c

@@ -2565,8 +2564,7 @@ void func_80B59A80(EnZl3* this, PlayState* play) {
 }

 void func_80B59AD0(EnZl3* this, PlayState* play) {
-    // todo look into
-    Actor* thisx = &this->actor; // unused, necessary to use 'this' first to fix regalloc
+    if (this) {}



Highlighting this for others to see

What's the most fake of the two?

this function taking a base actor doesnt really make sense, unless all the other functions that lead up to this can.
its possible that they copy pasted the signature from a main function i guess??

it is interestingly the only function that gets called with both this and play in func_80B55444. that fact doesnt really mean much, but its interesting

src/overlays/actors/ovl_En_Ik/z_en_ik.c

include/sched.h

src/code/audio_synthesis.c

Co-authored-by: Dragorn421 <Dragorn421@users.noreply.github.com>

Roman971

Even after giving it more thought I really don't think we should have UNUSED for arguments, since it's debatable and doesn't do much for us (besides making changes more difficult), plus it's incomplete. Also I think we at least need to figure out its impact on multi-version support and how we would deal with it.

I'm also not in favor of STACK_PAD/STACK_PADS, tho I'm not really opposed to those, but I have similar concerns regarding multi-version support. For instance, we need to figure out if we're okay with adding a bunch of version ifdefs in variable declarations just for the purpose of these macros (because that's almost certainly going to happen).

Dragorn421 · 2022-11-14T19:20:27Z

Could you demonstrate how UNUSED for arguments makes changes difficult?

I don't look forward to ifdef hell in stack variables either, but I don't see how stack padding being a macro would worsen it compared to a regular temp declaration

Roman971 · 2022-11-14T19:45:06Z

Could you demonstrate how UNUSED for arguments makes changes difficult?

Having UNUSED in arguments mostly just adds more stuff to keep track of, especially since a distinction is made between prototypes and actual function declarations (one having UNUSED while the other doesn't). So for instance if you're updating a function's arguments and/or name you might copy-paste it from the C file to update the header, but then you have to remember to remove the UNUSED while doing that (and we have to look for that in reviews). There are other minor consequences like modders having to remove the UNUSED if they want to use the argument, which can also lead to git conflicts for example. It's pretty small stuff overall tho, but it's an additional reason for me to dislike it.

I don't look forward to ifdef hell in stack variables either, but I don't see how stack padding being a macro would worsen it compared to a regular temp declaration

For example if you have a variable which is used in some versions but not others, it could normally be handled by just having it declared in all versions, and only used in some (maybe with a comment like s32 count; // unused in non-debug). But with these stack pad macros we would instead need an ifdef (with one side using the STACK_PAD and the other being the variable). And it could get really messy if multiple variables are in similar cases within the same block declaration.
Ofc there are cases where the stack layout will be straight up incompatible and we will need ifdefs anyway, but requiring STACK_PAD necessarily means we have less options at our disposal.

Dragorn421 · 2022-11-14T20:02:14Z

Inconsistency between function declaration and definition due to UNUSED is a concern I can understand, but I don't think it's a big deal. It's easy to catch and not highly serious

I don't think removing UNUSED if using an argument is an inconvenience at all, and causing conflicts from that should not be a concern compared to conflicts that would arise from changing anything else

I didn't think of the case of a temp that would be used in only some versions, making it stack padding in others. I have no idea if that would be a rare occurrence or not
Even if it was common though, it's easy to revert a STACK_PAD macro to temps, but then the temp being "partly" unused makes UNUSED sketchy... as a solution, we may resort to classic (void)temp; then, but idk

Roman971 · 2022-11-14T20:12:33Z

I didn't think of the case of a temp that would be used in only some versions, making it stack padding in others. I have no idea if that would be a rare occurrence or not

I think this is going to be common, especially considering the amount of versions that have some features and not others (debug and ntsc vs pal), but I could be wrong on that.

Also it's worth noting that this scenario could happen with arguments, and it's going to get really ugly if we have to ifdef inside function prototypes just to include/exclude UNUSED in one or even multiple arguments.

Even if it was common though, it's easy to revert a STACK_PAD macro to temps, but then the temp being "partly" unused makes UNUSED sketchy... as a solution, we may resort to classic (void)temp; then, but idk

Since this PR enables unused warnings by default, it wouldn't be an option to not use STACK_PAD (or UNUSED), and afaik you can't have UNUSED on a variable that is actually used (at least I would expect the compiler to error on that). So I think we would always need some form of ifdef to avoid the warning.

Dragorn421 · 2022-11-14T20:18:32Z

To make things clear, UNUSED on a variable that is actually used is perfectly fine.
So you wouldn't need to ifdef around it, but I understand it's not appealing to put an UNUSED attribute on something that is used (sometimes)
And again (void)temp; is an option (not one I'm a fan of but it's there)

Roman971 · 2022-11-14T20:23:42Z

Wait really? GCC doesn't error or even warn if a variable declared unused is actually used?
That makes it so much worse than it already was in mind lol

Dragorn421 · 2022-11-14T20:30:22Z

Yes, here's a fantastic one-liner for checking the behavior

echo 'int main(){ int a; __attribute__((unused)) int b; int c; c=0; __attribute__((unused)) int d; d=0; }' | gcc -x c -Wunused-variable -fsyntax-only -

Dragorn421 · 2022-11-30T09:35:16Z

Status update on this PR:

Limbo

Would be good to hear more opinions. (even if there was no discussion, we'd still need a contributor approval)

Personally I understand Roman's concerns on ifdef'ing around unused stack variables, and it's bad we have zero visibility on how common/uncommon this scenario would be
(but I would still like to go ahead with merging this)

@Roman971 to put it simply do you "veto" this PR

fig02 · 2022-11-30T12:21:06Z

think we prob just need more opinions

so far 2 (3 including tharo) people are ok with this vs 1 who is not.
elliptic did say he thinks its too early to do, but that was a while ago and should prob say if that is still his opinion

Dragorn421 · 2023-01-21T16:45:02Z

Linking to a short discussion about this PR to not lose it:

https://discord.com/channels/688807550715560050/752770632357511239/1062683481378930730
to
https://discord.com/channels/688807550715560050/752770632357511239/1062903374938378240

Thar0 added 3 commits July 5, 2022 03:01

UNUSED attribute for variables and parameters in static segments and …

950b8b0

…for all stack padding variables

No more unused variable warnings

1769153

Make pad variable names consistent

c6f2ee9

Dragorn421 reviewed Jul 12, 2022

View reviewed changes

Merge branch 'master' of https://github.com/zeldaret/oot into unused

9554a45

fig02 added the Needs discussion There is a debate or other required discussion preventing changes from being made label Aug 27, 2022

Dragorn421 added Needs contributor approval labels Sep 30, 2022

Thar0 added 5 commits October 30, 2022 01:31

Merge branch 'master' of https://github.com/zeldaret/oot into unused

6ea552a

Small cleanup

80102d1

Macro-ify stack padding

d291784

Use stack padding for more variables not previously named pad

12c613e

Format

a9b8ca8

Dragorn421 reviewed Oct 30, 2022

View reviewed changes

Makefile Outdated Show resolved Hide resolved

include/functions.h Outdated Show resolved Hide resolved

Check unused parameter on gamestate overlays and pause menu

65033c5

Co-authored-by: Dragorn421 <Dragorn421@users.noreply.github.com>

Dragorn421 reviewed Oct 30, 2022

View reviewed changes

Thar0 added 3 commits November 1, 2022 19:01

Merge 2x u8 stack pads into 1x s16 stack pad in en_ik

b23e049

Remove UNUSED from function prototypes

6e4c531

Merge branch 'master' of https://github.com/zeldaret/oot into unused

5eb18ee

Dragorn421 reviewed Nov 3, 2022

View reviewed changes

include/sched.h Outdated Show resolved Hide resolved

src/code/audio_synthesis.c Outdated Show resolved Hide resolved

Suggested changes

9846caa

Co-authored-by: Dragorn421 <Dragorn421@users.noreply.github.com>

Dragorn421 approved these changes Nov 3, 2022

View reviewed changes

fig02 approved these changes Nov 4, 2022

View reviewed changes

Dragorn421 requested a review from Roman971 November 10, 2022 02:10

Roman971 reviewed Nov 14, 2022

View reviewed changes

fig02 removed Needs contributor approval labels Aug 27, 2023

Thar0 added 2 commits September 7, 2023 20:40

Merge remote-tracking branch 'upstream/master' into unused

b94d4e4

More unused variables

72e2752

Dragorn421 mentioned this pull request Nov 7, 2023

Struct Returns #1574

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unused variable/parameter cleanup #1321

Unused variable/parameter cleanup #1321

Thar0 commented Jul 9, 2022

Dragorn421 left a comment

EllipticEllipsis commented Jul 12, 2022 •

edited

Thar0 commented Jul 29, 2022

fig02 commented Jul 29, 2022

Roman971 commented Jul 29, 2022

Dragorn421 commented Jul 29, 2022

Thar0 commented Jul 29, 2022

Dragorn421 commented Jul 29, 2022

Dragorn421 commented Oct 7, 2022

Thar0 commented Oct 22, 2022

Dragorn421 commented Oct 22, 2022

Dragorn421 left a comment

Dragorn421 Oct 30, 2022

Dragorn421 Oct 30, 2022

fig02 Oct 31, 2022

Roman971 left a comment

Dragorn421 commented Nov 14, 2022 •

edited

Roman971 commented Nov 14, 2022 •

edited

Dragorn421 commented Nov 14, 2022

Roman971 commented Nov 14, 2022

Dragorn421 commented Nov 14, 2022

Roman971 commented Nov 14, 2022

Dragorn421 commented Nov 14, 2022

Dragorn421 commented Nov 30, 2022 •

edited

fig02 commented Nov 30, 2022

Dragorn421 commented Jan 21, 2023

Unused variable/parameter cleanup #1321

Are you sure you want to change the base?

Unused variable/parameter cleanup #1321

Conversation

Thar0 commented Jul 9, 2022

Dragorn421 left a comment

Choose a reason for hiding this comment

EllipticEllipsis commented Jul 12, 2022 • edited

Thar0 commented Jul 29, 2022

fig02 commented Jul 29, 2022

Roman971 commented Jul 29, 2022

Dragorn421 commented Jul 29, 2022

Thar0 commented Jul 29, 2022

Dragorn421 commented Jul 29, 2022

Dragorn421 commented Oct 7, 2022

Thar0 commented Oct 22, 2022

Dragorn421 commented Oct 22, 2022

Dragorn421 left a comment

Choose a reason for hiding this comment

Dragorn421 Oct 30, 2022

Choose a reason for hiding this comment

Dragorn421 Oct 30, 2022

Choose a reason for hiding this comment

fig02 Oct 31, 2022

Choose a reason for hiding this comment

Roman971 left a comment

Choose a reason for hiding this comment

Dragorn421 commented Nov 14, 2022 • edited

Roman971 commented Nov 14, 2022 • edited

Dragorn421 commented Nov 14, 2022

Roman971 commented Nov 14, 2022

Dragorn421 commented Nov 14, 2022

Roman971 commented Nov 14, 2022

Dragorn421 commented Nov 14, 2022

Dragorn421 commented Nov 30, 2022 • edited

fig02 commented Nov 30, 2022

Dragorn421 commented Jan 21, 2023

EllipticEllipsis commented Jul 12, 2022 •

edited

Dragorn421 commented Nov 14, 2022 •

edited

Roman971 commented Nov 14, 2022 •

edited

Dragorn421 commented Nov 30, 2022 •

edited