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

macOS 14 SDK has a problematic use of __has_extension in TargetConditionals.h #9832

Closed
wants to merge 1 commit into from

Conversation

sezero
Copy link
Contributor

@sezero sezero commented May 18, 2024

Fixes #9632

CC: @Traace , @slime73

@sezero sezero requested review from icculus and slouken May 18, 2024 17:50
@sezero
Copy link
Contributor Author

sezero commented May 18, 2024

P.S.: If this is accepted, you may want to backport it to SDL2 branch.

# define __has_extension(x) 0
# endif
# define SDL_undef_has_extension
#endif /* __has_extension */
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it makes more sense to just include TargetConditionals.h in both branches rather than define a separate define to undefine.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Here's a proposed patch

diff --git a/include/SDL3/SDL_platform_defines.h b/include/SDL3/SDL_platform_defines.h
index 72b1ef260..b418ca411 100644
--- a/include/SDL3/SDL_platform_defines.h
+++ b/include/SDL3/SDL_platform_defines.h
@@ -65,7 +65,17 @@
 #define SDL_PLATFORM_APPLE  1
 /* lets us know what version of macOS we're compiling on */
 #include <AvailabilityMacros.h>
+/* The macOS 14 SDK uses __has_extension in TargetConditionals.h */
+#ifndef __has_extension
+# ifdef __has_feature
+#  define __has_extension __has_feature
+# else
+#  define __has_extension(x) 0
+# endif
 #include <TargetConditionals.h>
+#else
+#include <TargetConditionals.h>
+#endif

 /* Fix building with older SDKs that don't define these
    See this for more information:

Copy link
Collaborator

Choose a reason for hiding this comment

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

Are we sure that __has_extension and __has_feature are equivalent? Have we heard back from the original bug reporter to find out what compiler they're using?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it makes more sense to just include TargetConditionals.h in both branches rather than define a separate define to undefine.

The undefine was suggested by @icculus in #9632 (and I kind'a agree with it)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are we sure that __has_extension and __has_feature are equivalent?

They aren't exactly the same, but, as I said, that's what clang docs seem to suggest to use if __has_extension is absent: https://clang.llvm.org/docs/LanguageExtensions.html (search __has_extension on that page)

Have we heard back from the original bug reporter to find out what compiler they're using?

Nope. That's why I CC'd him here

Copy link
Contributor Author

@sezero sezero May 18, 2024

Choose a reason for hiding this comment

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

find out what compiler they're using?

However, I do think they're using a version of gcc: all versions of clang I tried do have __has_extension. As for gcc, gcc-10 doesn't have either of __has_extension and __has_feature, however gcc-14 has both of them (EDIT: the features do seem to be added in gcc14: compare https://gcc.gnu.org/onlinedocs/gcc-13.2.0/cpp/Conditional-Syntax.html and https://gcc.gnu.org/onlinedocs/gcc-14.1.0/cpp/Conditional-Syntax.html).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Rebased and simplified the patch a bit.

@slouken
Copy link
Collaborator

slouken commented May 27, 2024

@icculus and I chatted and we agreed we prefer separate include paths rather than the tap dance you have here. I'll go ahead and push that instead. Thanks for doing the legwork here!

@slouken slouken closed this May 27, 2024
@sezero sezero deleted the bug9632-macos14 branch May 27, 2024 21:33
@sezero
Copy link
Contributor Author

sezero commented May 27, 2024

OK. Please apply to SDL2, as well.

@slouken
Copy link
Collaborator

slouken commented May 27, 2024

Done!

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.

Latest macOS system header causes compilation failures
2 participants