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

Fix GetMonData and GetBoxMonData signatures #3170

Closed
wants to merge 1 commit into from

Conversation

gruxor
Copy link

@gruxor gruxor commented Jul 22, 2023

Description

Fixes the function signatures for GetMonData and GetBoxMonData. Thanks Gamefreak.

I did most of the changes with a pretty strict regex, but I skimmed through the changes for potential errors and didn't see any. There's one actual change in here that isn't just changing the signatures and adding NULL in place of the data buffer and it's at line 415 of daycare.c, where there was actually a bug that this change caught already (DaycareMon passed in, instead of BoxMon - no real effect since it was just a zero/null check, though).

There are probably "better" solutions to this problem, but this is the only one that doesn't involve unions, macros, creating additional/differently typed functions, or other widespread refactors. Might be worth looking into alternatives eventually, when doing other refactors, but this is a safety, usability, and clarity improvement in the meantime.

Discord contact info

crater.

@mrgriffin
Copy link
Collaborator

mrgriffin commented Jul 23, 2023

I think it would be more downstream-friendly to define GetMonData as a variadic macro. As-is this breaks everybody's builds if they ever called GetMonData, and breaks all feature branches doing the same. It's not a huge breakage, but neither does it really seem worth it.

Related is pret#1756, where the discussion in pret at the time was that it's not actually an improvement.

E: I have updated the PR on pret. This is the old implementation.

@gruxor
Copy link
Author

gruxor commented Jul 23, 2023

Yeah I don't feel too married to it, just figured it'd be the best baseline for getting error checking and eliminating some potentially wonky pitfalls, plus eliminating odd issues with intellisense/debuggers/anaylzers/etc. without overcomplicating things. I did figure that not worrying about matching would give us some more flexibility over pret for better solving this in the future, but that's about it.

I do think that any downstream repos shouldn't really have any trouble with this though - the compiler will provide exact locations and from there it'll be pretty obvious what's going on. Plus whatever analyzers/IDEs/plugins/etc. will catch it even before the compiler does. It's solved with a pretty simple find and replace, in any case, and that can be applied globally throughout the project (it's how I made this PR, even). But like I said, not really married to this solution either, so don't feel like that's a strong contention or anything.

Another idea, if backwards compatibility is the concern - we might be able to rename the existing functions, re-add the current jank ones, and then mark those with __attribute__((deprecated)) or something? Maybe something similar could be possible with the macros from the pret thread, or similar approaches, but I don't know (and there might not even be much value in that).

My ultimate goals were 1) not abstracting it behind a complex macro and 2) making sure the compiler can catch potential issues. Other than that, not picky about what kind of solution we use.

@gruxor gruxor closed this Jul 24, 2023
@gruxor gruxor deleted the upcoming-signature-fix branch July 24, 2023 14:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants