oot
oot copied to clipboard
zeldaret
Unused variable/parameter cleanup
- Applies the
UNUSEDattribute to unused variables, enables checking-Wunused-variablewarnings now that they're all suppressed. - Applies the
UNUSEDattribute 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-parameterwarnings are suppressed, I've left-Wno-unused-parameterinCHECK_WARNINGS. - Changes the names of variables starting with
padthat 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
padand for functions with multiple they are namedpad1,pad2, etc. Also renamed some padding variables with the same name in different scopes.
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.
(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)
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
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? 😰
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 :sunglasses:
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.
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" ? :sunglasses:
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 https://github.com/zeldaret/oot/pull/1321#pullrequestreview-1035882290 )
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)
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.
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
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
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.
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
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.
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)
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
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 -
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
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
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