Bug 108132 – Refactor the way HAVE_XXX macros are set

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Laszlo Gombos]

It seems to me that OS(WINDOWS) and OS(UNIX) are so broadly defined that for each builds/ports exactly one of them is enabled.

This opens up a door for re-factoring the fallback "#else" block to make the OS() guards always explicit in Platform.h (see /* FIXME: is this actually used or do other platforms generate their own config.h? */ ).

[Laszlo Gombos]

Created attachment 185101 [details]
proposed refactoring

I also flattened out the nested guards because I find it more readable. If others feel that this is not the case I am happy to keep the nesting as it is now.

[Laszlo Gombos]

Created attachment 185130 [details]
rebaseline

[Benjamin Poulain]

Comment on attachment 185130 [details]
rebaseline

View in context: https://bugs.webkit.org/attachment.cgi?id=185130&action=review

Unless I missed a HAVE_DISPATCH_H, this is gonna break iOS.

> Source/WTF/wtf/Platform.h:725
> -#if PLATFORM(IOS) || __MAC_OS_X_VERSION_MIN_REQUIRED >= 1060
> -
> +#if OS(DARWIN) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 1060
>  #define HAVE_DISPATCH_H 1
>  #define HAVE_HOSTED_CORE_ANIMATION 1

This will break iOS.

Do we still need __MAC_OS_X_VERSION_MIN_REQUIRED >= 1060 actually?

[Laszlo Gombos]

Created attachment 186709 [details]
fix the issue pointed out in the review

[Laszlo Gombos]

(In reply to comment #3)
> (From update of attachment 185130 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=185130&action=review

Thanks for the review.
 
> Unless I missed a HAVE_DISPATCH_H, this is gonna break iOS.

Great catch ! Fixed.
 
> Do we still need __MAC_OS_X_VERSION_MIN_REQUIRED >= 1060 actually?

Had the same question, so I filed bug 107964 a while back. I think the version work should be done in a separate patch.

[Benjamin Poulain]

Comment on attachment 186709 [details]
fix the issue pointed out in the review

View in context: https://bugs.webkit.org/attachment.cgi?id=186709&action=review

I am not convinced you are making things simpler when splitting blocks.

E.g.:
#if PLATFORM(IOS) || __MAC_OS_X_VERSION_MIN_REQUIRED >= 1060
Foo
Bar
#endif

#if OS(DARWIN) && !OS(IOS) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 1060
MacFoobar
#endif

Against:

#if PLATFORM(IOS) || (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 1060)
Foo
Bar
#if !PLATFORM(IOS)
MacFooBar
#endf // !PLATFORM(IOS)
#endif

> Source/WTF/wtf/Platform.h:687
>  #endif

#endif // OS(UNIX)

> Source/WTF/wtf/Platform.h:721
> +#endif

#endif // OS(DARWIN)

> Source/WTF/wtf/Platform.h:723
> +#if OS(IOS) || (OS(DARWIN) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 1060)

It is probably better to avoid OS(IOS) until Dave decided the fate of PLATFORM(MAC) and PLATFORM(IOS).


This?
#if PLATFORM(IOS) || __MAC_OS_X_VERSION_MIN_REQUIRED >= 1060
or 
#if PLATFORM(IOS) || (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 1060)

Having DARWIN and __MAC_OS_X_VERSION_MIN_REQUIRED together is not adding readability because iOS is also a Darwin system.

> Source/WTF/wtf/Platform.h:730
> +#if OS(DARWIN) && !OS(IOS) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 1060

PLATFORM(IOS)

[Laszlo Gombos]

Created attachment 187457 [details]
for cq

[Early Warning System Bot]

Comment on attachment 187457 [details]
for cq

Attachment 187457 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/16478560

[Early Warning System Bot]

Comment on attachment 187457 [details]
for cq

Attachment 187457 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/16469656

[EFL EWS Bot]

Comment on attachment 187457 [details]
for cq

Attachment 187457 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/16479553

[Laszlo Gombos]

Created attachment 187458 [details]
cq 2nd try

[WebKit Review Bot]

Comment on attachment 187458 [details]
cq 2nd try

Clearing flags on attachment: 187458

Committed r142399: <http://trac.webkit.org/changeset/142399>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Laszlo Gombos]

(In reply to comment #6)
> (From update of attachment 186709 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=186709&action=review
 
Thanks for the review Benjamin !

> It is probably better to avoid OS(IOS) until Dave decided the fate of PLATFORM(MAC) and PLATFORM(IOS).

Ok, will do. For future discussions, I though we have consensus on preferring OS() macros over PLATFORM() macros in general.

> This?
> #if PLATFORM(IOS) || __MAC_OS_X_VERSION_MIN_REQUIRED >= 1060
> or 
> #if PLATFORM(IOS) || (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 1060)
> 
> Having DARWIN and __MAC_OS_X_VERSION_MIN_REQUIRED together is not adding readability because iOS is also a Darwin system.

(__MAC_OS_X_VERSION_MIN_REQUIRED >= 1060) test needs either an OS(DARWIN) or a PLATFORM(MAC) guard around, otherwise the build fails on e.g. Qt Linux port because __MAC_OS_X_VERSION_MIN_REQUIRED is undefined.