[webkit-reviews] review granted: [Bug 108132] Refactor the way HAVE_XXX macros are set : [Attachment 186709] fix the issue pointed out in the review

[bugzilla-daemon at webkit.org]

Benjamin Poulain <benjamin at webkit.org> has granted Laszlo Gombos
<laszlo.gombos at webkit.org>'s request for review:
Bug 108132: Refactor the way HAVE_XXX macros are set
https://bugs.webkit.org/show_bug.cgi?id=108132

Attachment 186709: fix the issue pointed out in the review
https://bugs.webkit.org/attachment.cgi?id=186709&action=review

------- Additional Comments from Benjamin Poulain <benjamin at webkit.org>
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)