-Wundef errors from clang-11.1 when targeting macOS

[sezero]

Targeting i386 against 10.8 SDK:

In file included from src/SDL_assert.c:21:
In file included from src/./SDL_internal.h:52:
In file included from include/SDL_config.h:33:
include/SDL_platform.h:73:5: error: 'TARGET_OS_TV' is not defined, evaluates to 0 [-Werror,-Wundef-prefix=TARGET_OS_]
#if TARGET_OS_TV
    ^
1 error generated.

src/joystick/iphoneos/SDL_mfijoystick.m:38:5: error: 'TARGET_OS_IOS' is not defined, evaluates to 0 [-Werror,-Wundef-prefix=TARGET_OS_]
#if TARGET_OS_IOS
    ^
src/joystick/iphoneos/SDL_mfijoystick.m:460:5: error: 'TARGET_OS_TV' is not defined, evaluates to 0 [-Werror,-Wundef-prefix=TARGET_OS_]
#if TARGET_OS_TV
    ^
2 errors generated.

src/filesystem/cocoa/SDL_sysfilesystem.m:83:6: error: 'TARGET_OS_TV' is not defined, evaluates to 0 [-Werror,-Wundef-prefix=TARGET_OS_]
#if !TARGET_OS_TV
     ^
1 error generated.

Targeting x86_64 against 10.12 SDK:

src/video/SDL_video.c:1492:25: error: 'TARGET_OS_MACCATALYST' is not defined, evaluates to 0 [-Werror,-Wundef-prefix=TARGET_OS_]
#if SDL_VIDEO_METAL && (TARGET_OS_MACCATALYST || __MACOSX__ || __IPHONEOS__)
                        ^
1 error generated.

An easy solution would be defining those offenders as 0 if they
are not defined, like below. Is it correct?

diff --git a/include/SDL_platform.h b/include/SDL_platform.h
index ebaa2524..88f5e5a8 100644
--- a/include/SDL_platform.h
+++ b/include/SDL_platform.h
@@ -68,10 +68,22 @@
 
 #if defined(__APPLE__)
 /* lets us know what version of Mac OS X we're compiling on */
 #include "AvailabilityMacros.h"
 #include "TargetConditionals.h"
+#ifndef TARGET_OS_MACCATALYST
+#define TARGET_OS_MACCATALYST 0
+#endif
+#ifndef TARGET_OS_IOS
+#define TARGET_OS_IOS    0
+#endif
+#ifndef TARGET_OS_IPHONE
+#define TARGET_OS_IPHONE 0
+#endif
+#ifndef TARGET_OS_TV
+#define TARGET_OS_TV 0
+#endif
 #if TARGET_OS_TV
 #undef __TVOS__
 #define __TVOS__ 1
 #endif
 #if TARGET_OS_IPHONE

[icculus]

Did we enable this warning? We should disable it, it looks like a bad warning.

[sezero]

Did we enable this warning?

No we did not.

Apparently LLVM/Clang 11 automatically adds -Werror,-Wundef-prefix=TARGET_OS_
Pointers from a Google search:
https://www.google.com/search?q=clang+error+%22-Werror%2C-Wundef-prefix%3D%E2%80%8BTARGET_OS_%22

it looks like a bad warning.

Be that as it may, 3rd party projects may compile with -Wundef against
SDL headers, so I think we should fix it.

[icculus]

Oh, this is a public header, I misread, sorry.

I would say we shouldn’t define these things, since that will cause problems for apps that checked these with #ifndef, as we should too.

[sezero]

Oh, this is a public header, I misread, sorry.

Not only it's a public header, but SDL's own build is affected (as I noted above.)

I would say we shouldn’t define these things, since that will cause problems for apps that checked these with #ifndef, as we should too.

So we should do something like the following, yes?

---#if TARGET_OS_TV
+++#if defined(TARGET_OS_TV) && (TARGET_OS_TV != 0)

(It will possibly be a bit intrusive, though.)

[sezero]

Easy solution is #pragma clang diagnostic ignored "-Wundef-prefix"
coupled with #pragma clang diagnostic ignored "-Wunknown-warning-option"
Fixes my local builds targeting MacOSX.

@icculus and/or @slouken: Is this attached patch OK? clang11-patch.txt

[icculus]

So we should do something like the following, yes?

I think we only need to check if it’s defined, not that it is non-zero, right? If so, that’s less intrusive that the pragma changes.

[okuoku]

SDK 10.8 just predates tvOS thus checking defined first should be way to go. Please don't try to define TARGET_ macros by ourselves since it can pollute user's namespace as one can doing ifdef TARGET_OS_TV to detect SDK version(despite it's not the right way)...

I think we only need to check if it’s defined, not that it is non-zero, right?

On macOS/iOS SDKs, every target macros will be defined that expand to 0 or 1 so always do full check #if defined(TARGET_OS_TV) && (TARGET_OS_TV != 0).

[sezero]

I think we only need to check if it’s defined, not that it is non-zero, right?

On macOS/iOS SDKs, every target macros will be defined that expand to 0 or 1

Indeed, that is the wrong way

so always do full check #if defined(TARGET_OS_TV) && (TARGET_OS_TV != 0).

That covers much more volume than the pragma changes I suggested:
The existing checks are actually correct IMO, we only want to by-pass the
overzealous behavior of clang.

[sezero]

Updated patch (small revisions) is attached: clang11-patch2.txt

(The changes to uikit/* parts may be omitted if it is guaranteed
not to be affected: can't test those myself.)

Can you guys give me a decision?

[slouken]

I think the original suggestion is the correct fix.
Those defines should be present after the target conditionals header, and if they haven't been defined they should be treated as 0
It also makes all the other code much less messy.

[slouken]

The only issue would be if there's an SDK combination where something is undefined, but assumed to be true, which might be the case at different SDK versions.

There's some good discussion of this here:
https://stackoverflow.com/questions/12132933/preprocessor-macro-for-os-x-targets

[sezero]

I think the original suggestion is the correct fix.
Those defines should be present after the target conditionals header,
and if they haven't been defined they should be treated as 0
It also makes all the other code much less messy.

That would be ideal, although @icculus and @okuoku present arguments
against it.

The only issue would be if there's an SDK combination where something
is undefined, but assumed to be true, which might be the case at different
SDK versions.

There's some good discussion of this here:
https://stackoverflow.com/questions/12132933/preprocessor-macro-for-os-x-targets

Ugh. Eh.. This is a mess.. Do you have a patch?
Or what do I do?

[sezero]

It would be nice to fix this before 2.0.16 ships.

[slouken]

Agreed. It's on the 2.0.16 milestone and Ryan and I will figure out what we want to do.

In the meantime, would you be able to figure out if there are any of those definitions that need to be true based on SDK version and other preprocessor defines?

[sezero]

Sam: Note that I only have the MacOSX versions of the SDKs.
Here is what I see in there:
TARGET_OS_IPHONE - starts with 10.6
TARGET_OS_SIMULATOR - starts with 10.11 (deprecating TARGET_IPHONE_SIMULATOR)
TARGET_OS_IOS, TARGET_OS_TV and TARGET_OS_WATCH - starts with 10.11
TARGET_OS_MACCATALYST - starts with 10.15
TARGET_OS_BRIDGE - appears in 10.12/13, not defined as of 10.14

[sezero]

PING?