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 #if *_ENABLED inconsistencies, should check if defined #87286

Merged

Conversation

akien-mga
Copy link
Member

Redo and extension of #87272 for the master branch.
See #87272 (comment) for rationale.

This shouldn't change behavior, but makes things consistent with the rest of the codebase and less error prone.

@akien-mga akien-mga added this to the 4.3 milestone Jan 17, 2024
@akien-mga akien-mga requested review from a team as code owners January 17, 2024 09:14
Co-authored-by: Caroline Joy Bell <halotroop2288@proton.me>
@akien-mga akien-mga force-pushed the fix-preprocessor-if-ENABLED-checks branch from 33a7e8a to 0a7579b Compare January 17, 2024 09:30
"Windows": "#ifdef WINDOWS_ENABLED",
"Mac OS X": "#ifdef MACOS_ENABLED",
"Android": "#if defined(__ANDROID__)",
"Android": "#ifdef ANDROID_ENABLED",
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added this, which is the only functional change in this PR.
I couldn't see a reason why this one used a compiler-provided macro instead of our own like other platforms.

I guess at the time it was authored we might not have had an ANDROID_ENABLED define yet? Or it was just an oversight.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did a quick blame trip, seems like this was added in 40b0c55 as is, but that was copied from the defines used at the time in main/input_default.cpp. And those date back to af633c7, which already had the ANDROID_ENABLED define, so I think this was just an oversight.

Copy link
Contributor

@YuriSizov YuriSizov Jan 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

which already had the ANDROID_ENABLED define

Well, we had it since GODOT IS OPEN SOURCE, so I think your assessment is correct. It's strange, because it's inconsistent, but you'd have to ask Ariel about it, because it originates from 0d80726. 🙃

@YuriSizov YuriSizov merged commit b6a2c78 into godotengine:master Jan 17, 2024
15 checks passed
@YuriSizov
Copy link
Contributor

Thanks!

@akien-mga akien-mga mentioned this pull request Jan 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants