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 SCU global namespace conflict in resource_format_text.cpp #96511

Merged
merged 1 commit into from
Sep 3, 2024

Conversation

lawnjelly
Copy link
Member

@lawnjelly lawnjelly commented Sep 3, 2024

FORMAT_VERSION is used in multiple places in the codebase, and #defining it was causing conflicts.

Notes

  • I hadn't built master in quite a few days, am surprised no one else was getting this conflict.
  • This likely only occurs in SCU build, as other files attempt to add FORMAT_VERSION in enums after resource_format_text.cpp has #defined it.
  • Everywhere else seems to more sensibly use FORMAT_VERSION in local enums, which is safer for making sure the correct version is used.
  • FORMAT_VERSION_COMPAT and _printerr() weren't causing build errors for me, but seemed worth fixing at the same time.

In general #define should be used with care due to the potential for conflicts with global namespace (and the use of contrived naming may be recommended for defines). This applies not just in SCU builds.

The alternative here would have been to exclude the problematic .cpp from the SCU build in scene/resources/SCsub, but in this case it looks more sensible to fix the unnecessary use of global namespace, which may fix more bugs in the long run.

`FORMAT_VERSION` is used in multiple places in the codebase, and #defining it was causing conflicts.
@akien-mga akien-mga added the bug label Sep 3, 2024
@akien-mga akien-mga added this to the 4.4 milestone Sep 3, 2024
@akien-mga akien-mga merged commit 9dbf9aa into godotengine:master Sep 3, 2024
19 checks passed
@akien-mga
Copy link
Member

Thanks!

@lawnjelly lawnjelly deleted the fix_namespace_conflict branch September 3, 2024 10:35
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