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

Declarations for 2- and 3-argument GetMonData #1756

Merged
merged 2 commits into from
Jul 24, 2023

Conversation

mrgriffin
Copy link
Collaborator

We definitely shouldn't name the functions GetMonData and GetMonData2, so this can't be merged without somebody coming up with a better suggestion.

Also, Rose on Discord suggested having something that wraps GetMonData for constant MON_DATA_* arguments, which might be nice (although the ones that are used both with GetMonData and GetMonData2 would end up being pretty ugly, so perhaps that's a nicer dream than reality).

I think this should match, but just to be sure I've opened a PR so that the CI system can tell me if it doesn't.

@tustin2121
Copy link
Contributor

Might I suggest GetMonData and GetMonData_Param?
Or perhaps GetMonData_CopyOut or _Out, or something like that that specifies that the pointer added to the signature is an out parameter?

@GriffinRichards
Copy link
Member

Is this really an improvement? It seems more likely that someone would search for the definition of e.g. GetMonData2 and get confused when they only find a declaration (assuming they're not sure what the alias attribute is doing). The current declarations are bad practice but I'd think that causes people less grief than this would.

@mrgriffin
Copy link
Collaborator Author

mrgriffin commented Aug 26, 2022

It's an improvement in the sense that it catches errors at compile time. Currently you can GetBoxMonData() and get some very interesting results. Also, alias is a bit confusing, but so is a function that seems to take either 2 or 3 arguments; that's not a thing that's common in C (besides the printf/scanf family of functions).

But to be clear, I mostly did this to see if it was possible, it's up to others to push for its inclusion or not.

@mrgriffin mrgriffin marked this pull request as ready for review July 23, 2023 06:54
@mrgriffin
Copy link
Collaborator Author

Reviving this in response to rh-hideout#3170.

Much less invasive than before, all calls (except one erroneous one in src/daycare.c) have stayed the same, and instead we use a macro to choose between GetMonData2 and GetMonData3 based on the number of arguments.

I think this change does achieve type-safety, e.g. the call in src/daycare.c was "wrong". It worked, because DaycareMon starts with a BoxPokemon member, and so C guarantees that the previous code worked. But I think it's nicer to pass the BoxPokemon pointer directly?

@GriffinRichards
Copy link
Member

Could you put some similar comment in pokemon.c? I imagine some people will be confused when for example they can't find a definition of GetMonData in there, only GetMonData2 and GetMonData3.

@mrgriffin
Copy link
Collaborator Author

I've added some wording that I hope is useful :)

Feel free to squash this if you merge it.

@GriffinRichards GriffinRichards merged commit b53cca1 into pret:master Jul 24, 2023
1 check passed
Flametix pushed a commit to Flametix/pokeemerald that referenced this pull request Oct 2, 2023
* Type-safe GetMonData/GetBoxMonData

* Comments
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.

3 participants