Collapse PLATFORM(MAC)||PLATFORM(IOS) to just PLATFORM(MAC) as PLATFORM(IOS) is a specialization of PLATFORM(MAC). PLATFORM(MAC) is always enabled when PLATFORM(IOS) is enabled.
Created attachment 183694 [details] proposed change I found having both "PLATFORM(MAC) && !PLATFORM(IOS)" and "PLATFORM(MAC) || PLATFORM(IOS)" around to be slightly confusing.
(In reply to comment #1) > Created an attachment (id=183694) [details] > proposed change > > I found having both "PLATFORM(MAC) && !PLATFORM(IOS)" and "PLATFORM(MAC) || PLATFORM(IOS)" around to be slightly confusing. Defining PLATFORM(MAC) when building PLATFORM(IOS) was just a convenience for the iOS port since (more often than not) the Mac and iOS platforms share code. It's also a messy convenience because in a tree where iOS is merged with open source, the following macros have to be defined in these cases: Mac only: PLATFORM(MAC) && !PLATFORM(IOS) iOS only: PLATFORM(IOS) Mac or iOS: PLATFORM(MAC) __or__ PLATFORM(MAC) || PLATFORM(IOS) I see two problems with this: 1. Having to use "PLATFORM(MAC) && !PLATFORM(IOS)" when you only want to build code for Mac. 2. Reading "PLATFORM(MAC)" doesn't really evoke "this code is for both Mac and iOS". A better long-term solution would be to: - Not define PLATFORM(MAC) when building PLATFORM(IOS). - Move uses of "PLATFORM(MAC) || PLATFORM(IOS)" to either a USE()/ENABLE() macro or some third PLATFORM() macro that is defined for both platforms. What about doing this instead for the near term: - Stop defining PLATFORM(MAC) when building for iOS. - Add a new PLATFORM(IOS_MAC) macro for cases when the code works for both (with the understanding that we will want to use a different name later since we don't have a good name today). - Replace "PLATFORM(MAC) || PLATFORM(IOS)" with "PLATFORM(IOS_MAC)". - Replace "PLATFORM(MAC) && !PLATFORM(IOS)" with just "PLATFORM(MAC)". Thoughts?
I don't think we should block this patch because of future improvements. Most of the code already assume PLATFORM(MAC) implies PLATFORM(IOS). Other than that, I agree with your points. Maybe we could generalize OS(DARWIN) but I am afraid this will lead to many (OS(DARWIN) && !PLATFORM(QT)). Maybe - Replace "PLATFORM(MAC) || PLATFORM(IOS)" with "PLATFORM(IOS_MAC)". PLATFORM(APPLE_DARWIN)? PLATFORM(APPLE) - Replace "PLATFORM(MAC) && !PLATFORM(IOS)" with just "PLATFORM(MAC)". PLATFORM(OS_X)?
> Other than that, I agree with your points. > Maybe we could generalize OS(DARWIN) but I am afraid this will lead to many (OS(DARWIN) && !PLATFORM(QT)). > > Maybe > - Replace "PLATFORM(MAC) || PLATFORM(IOS)" with "PLATFORM(IOS_MAC)". > PLATFORM(APPLE_DARWIN)? > PLATFORM(APPLE) This is approach is most similar to the one we use for Chromium. We use PLATFORM(CHROMIUM) for the general approach and then use OS-specific macros in the cases where the details vary based on the operating system. You could also pick a more abstract name than PLATFORM(APPLE) if you might want to include Windows or other non-Apple operating systems in the future. For example, PLATFORM(TITANIUM) refers to a nice metal. :)
Two thoughts here: 1) Renaming PLATFORM(MAC) since it’s sort of a base platform for both Mac and iOS sounds like a pretty good idea. 2) A bigger problem the extensive use of PLATFORM(MAC) and similar in the code instead of other derived more specific conditionals. It’s often a bug to have #if PLATFORM(MAC) somewhere; but we settle for that more often than we should.
(In reply to comment #5) > Two thoughts here: > > 1) Renaming PLATFORM(MAC) since it’s sort of a base platform for both Mac and iOS sounds like a pretty good idea. > > 2) A bigger problem the extensive use of PLATFORM(MAC) and similar in the code instead of other derived more specific conditionals. It’s often a bug to have #if PLATFORM(MAC) somewhere; but we settle for that more often than we should. My longstanding theory is that the PLATFORM macros should be phased out entirely in favor of more specific ones. That may take some work to achieve though.
One of my motivations was to get a confirmation that _currently_ PLATFORM(IOS) assumes that PLATFORM(MAC) is defined so that I can continue to refactor the handling of ENABLE() defines. This is now confirmed. I tend to agree with Benjamin that while this patch does not make the situation a whole lot better at least it eliminates one of the ways to express "Mac or iOS". I think there needs to be a review of a standalone PLATFORM(MAC) guards before the situation can be significantly improved further.
(In reply to comment #7) > I tend to agree with Benjamin that while this patch does not make the situation a whole lot better at least it eliminates one of the ways to express "Mac or iOS". My concern remains that collapsing "PLATFORM(MAC) || PLATFORM(IOS)" into "PLATFORM(MAC)" leads to a loss of information (if only documentation that this code should be compiled for iOS) so that when we stop defining PLATFORM(MAC) when building for iOS, we now have to go back and re-audit any place that PLATFORM(IOS) was dropped. There aren't many now, but there will be many more in the future. What if we defined a multi-platform macro like this? #define ANY_PLATFORM_3(WTF_FEATURE1, WTF_FEATURE2, WTF_FEATURE3) (PLATFORM(WTF_FEATURE1) || PLATFORM(WTF_FEATURE2) || PLATFORM(WTF_FEATURE3)) #define ANY_PLATFORM(WTF_FEATURE1, WTF_FEATURE2) (PLATFORM(WTF_FEATURE1) || PLATFORM(WTF_FEATURE2)) #define PLATFORM(WTF_FEATURE) (defined WTF_PLATFORM_##WTF_FEATURE && WTF_PLATFORM_##WTF_FEATURE) Then "PLATFORM(MAC) || PLATFORM(IOS)" would become "ANY_PLATFORM(IOS, MAC)". And we could collapse actual logic like this in GraphicsContext3D.h: #if PLATFORM(MAC) || PLATFORM(GTK) || PLATFORM(QT) || PLATFORM(EFL) || PLATFORM(BLACKBERRY) to something like this: #if ANY_PLATFORM_5(BLACKBERRY, EFL, GTK, MAC, QT) One (big?) downside to this is that it becomes much harder to search for all "PLATFORM(FOO)" macros using a purely text search. Not sure how often that is done, though.
IMHO "ANY_PLATFORM(IOS, MAC)" is worse than PLATFORM(MAC) || PLATFORM(IOS)" because it's harder to search for and mysterious whereas the latter is at least obvious in what it means.
Comment on attachment 183694 [details] proposed change View in context: https://bugs.webkit.org/attachment.cgi?id=183694&action=review r- to incorporate suggested changes outside of Platform.h. > Source/JavaScriptCore/runtime/DatePrototype.cpp:65 > -#if PLATFORM(MAC) || PLATFORM(IOS) || (PLATFORM(WX) && OS(DARWIN)) || (PLATFORM(QT) && OS(DARWIN)) > +#if PLATFORM(MAC) || (PLATFORM(WX) && OS(DARWIN)) || (PLATFORM(QT) && OS(DARWIN)) > #include <CoreFoundation/CoreFoundation.h> This #if should probably be: #if USE(CF) However, this code would also now compile on the following platforms: PLATFORM(WIN) && !OS(WINCE) PLATFORM(CHROMIUM) && OS(DARWIN) The Chromium-on-Mac macro doesn't matter since Chromium uses V8. Not sure about !WinCE, though. > Source/JavaScriptCore/runtime/DatePrototype.cpp:128 > -#if PLATFORM(MAC) || PLATFORM(IOS) || (PLATFORM(WX) && OS(DARWIN)) || (PLATFORM(QT) && OS(DARWIN)) > +#if PLATFORM(MAC) || (PLATFORM(WX) && OS(DARWIN)) || (PLATFORM(QT) && OS(DARWIN)) Ditto about switching to #if USE(CF) here. > Source/WTF/wtf/Platform.h:473 > -#if PLATFORM(MAC) || PLATFORM(IOS) > +#if PLATFORM(MAC) I'm going to restate what's already been said, but we need to stop defining PLATFORM(MAC) when building PLATFORM(IOS) (and PLATFORM(IOS_SIMULATOR)) to make the decision explicit about whether a given piece of code supports both Mac and iOS, and to make Mac-only code not require "PLATFORM(MAC) && !PLATFORM(IOS)". I'll file a follow-up bug. The other thing I noticed about these uses of "PLATFORM(MAC) || PLATFORM(IOS)" in Platform.h is that' they're all defining other ENABLE() or USE() macros, which is exactly how they should be used since it makes it explicit about which platforms are defining each macro. To put it another way, using "PLATFORM(MAC) || PLATFORM(IOS)" in Platform.h is good, but using it outside of Platform.h means we're probably missing an ENABLE() or USE() macro. > Source/WebCore/config.h:148 > // CoreAnimation is available to IOS, Mac and Windows if using CG > -#if PLATFORM(MAC) || PLATFORM(IOS) || (PLATFORM(WIN) && USE(CG)) > +#if PLATFORM(MAC) || (PLATFORM(WIN) && USE(CG)) > #define WTF_USE_CA 1 > #endif This exact code is already in Source/WTF/wtf/Platform.h, and <wtf/Platform.h> is included in config.h above. It should just be removed.
(In reply to comment #10) > (From update of attachment 183694 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=183694&action=review > > > Source/WTF/wtf/Platform.h:473 > > -#if PLATFORM(MAC) || PLATFORM(IOS) > > +#if PLATFORM(MAC) > > I'm going to restate what's already been said, but we need to stop defining PLATFORM(MAC) when building PLATFORM(IOS) (and PLATFORM(IOS_SIMULATOR)) to make the decision explicit about whether a given piece of code supports both Mac and iOS, and to make Mac-only code not require "PLATFORM(MAC) && !PLATFORM(IOS)". I'll file a follow-up bug. I filed Bug 108007.
Thanks David for the review ! > > Source/JavaScriptCore/runtime/DatePrototype.cpp:65 > > -#if PLATFORM(MAC) || PLATFORM(IOS) || (PLATFORM(WX) && OS(DARWIN)) || (PLATFORM(QT) && OS(DARWIN)) > > +#if PLATFORM(MAC) || (PLATFORM(WX) && OS(DARWIN)) || (PLATFORM(QT) && OS(DARWIN)) > > #include <CoreFoundation/CoreFoundation.h> > > This #if should probably be: #if USE(CF) Filed bug to turn the guard to "OS(DARWIN) && USE(CF)" - see bug 108018. Filed a bug to get rid of "OS(DARWIN)" part and just use USE(CF) - see bug 108188 . Somewhat off topic - I also filed a bug for the Gtk Mac port to hit this code path - see bug 107492. > > Source/WebCore/config.h:148 > > // CoreAnimation is available to IOS, Mac and Windows if using CG > > -#if PLATFORM(MAC) || PLATFORM(IOS) || (PLATFORM(WIN) && USE(CG)) > > +#if PLATFORM(MAC) || (PLATFORM(WIN) && USE(CG)) > > #define WTF_USE_CA 1 > > #endif > > This exact code is already in Source/WTF/wtf/Platform.h, and <wtf/Platform.h> is included in config.h above. It should just be removed. Filed bug 108016 - it does not look like though that we can just remove these lines from config.h as that would break the WIN build. I think all the issues discussed here are now covered by the new bugs filed so I propose to close this bug.