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

Convert defines that could be represented as Enums into Enums #4611

Open
hedara90 opened this issue May 22, 2024 · 3 comments
Open

Convert defines that could be represented as Enums into Enums #4611

hedara90 opened this issue May 22, 2024 · 3 comments
Labels
feature-request Requests a new feature

Comments

@hedara90
Copy link
Collaborator

Description

Converting defines into enums where possible would enable the use of the -Wenum-enversion compiler option to detect cases where one this is being used for something it's not.
Some other compiler options already enabled with -Wall would also trigger on bad usage of enumified defines.
20240521_14h40m19s_grim
Currently the preproc used in the expansion can't handle enums in some parts of the code. A PR is available to fix this in the base pret pokeemerald, but the comments on that make some valid points regarding the preproc. So that would need to be addressed before the "enumification" could be done.
By merging that PR, replacing all #define TYPE_XXXX with an enum with all of them worked with no other changes for compiling the ROM, tests on the other hand failed to compile. It also fails to compile with agbcc.

Discord contact info

hedara

@hedara90 hedara90 added the feature-request Requests a new feature label May 22, 2024
@mrgriffin
Copy link
Collaborator

mrgriffin commented Jul 13, 2024

Just stating it here so we don't forget, imo anything that ends up being stored in the save should have explicit values, e.g. enum SpeciesID { SPECIES_NONE = 0, ... }; rather than just enum SpeciesID { SPECIES_NONE, ... };. This is because reordering the values would break saves, so having the friction of all the numbers being explicit will hopefully help discourage downstream users and well-meaning contributors from doing that by accident.

@AsparagusEduardo
Copy link
Collaborator

Just stating it here so we don't forget, imo anything that ends up being stored in the save should have explicit values, e.g. enum SpeciesID { SPECIES_NONE = 0, ... }; rather than just enum SpeciesID { SPECIES_NONE, ... };. This is because reordering the values would break saves, so having the friction of all the numbers being explicit will hopefully help discourage downstream users and well-meaning contributors from doing that by accident.

If that's the case, why even make them enums to begin with?
It might be clearer that constants stored in the save use #define while those that aren't use enum.

@mrgriffin
Copy link
Collaborator

mrgriffin commented Jul 13, 2024

If that's the case, why even make them enums to begin with?

-Wenum-conversion will give you an error if you pass an enum of the wrong type to a function/assign it to a variable.
EDIT: And -Wswitch (enabled by -Wall) will warn you if you have a switch (x) where x is an enum and your cases don't cover all the options.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-request Requests a new feature
Projects
None yet
Development

No branches or pull requests

3 participants