[webkit-dev] Removing BUILDING_ON / TARGETING macros in favor of system availability macros

Wed Jul 11 18:04:49 PDT 2012

On 2012-07-11, at 17:46, Mark Mentovai <mark at chromium.org> wrote:

> Tony brought me in to comment on what impact this might have on the Chromium Mac build. It shouldn’t have any impact. Any use of the compiler-defined macros is fine.

I agree. The only impact I expect this to have is if I've missed hand-fixing any cases that were using BUILDING_ON and meant it (vs the vast majority of cases where they really wanted TARGETING). There were around half a dozen instances of this that I noticed and fixed when building the Mac build against an SDK but it's possible some may exist in Chromium-specific code paths. These cases should cause build failures and be obvious to address.

> In Chrome code, we usually use MAC_OS_X_VERSION_MAX_ALLOWED and MAC_OS_X_VERSION_MIN_REQUIRED from <AvailabilityMacros.h>, along with symbolic macros like MAC_OS_X_VERSION_10_7 instead of 1070. This has an annoying disadvantage that older SDKs (like the 10.6 SDK) don’t define MAC_OS_X_VERSION_10_7, so we’re stuck testing things like defined(MAC_OS_X_VERSION_10_7) && MAC_OS_X_VERSION_MAX_ALLOWED >= MAC_OS_X_VERSION_10_7. The same applies when using <Availability.h>, where the symbolic names are of the form __MAC_10_7, but you seem to have chosen not to use those, thus condensing the macro logic.
> 
> We used <AvailabilityMacros.h>’s MAC_OS_X_VERSION_MAX_ALLOWED and MAC_OS_X_VERSION_MIN_REQUIRED instead of <Availability.h>’s __MAC_OS_X_VERSION_MAX_ALLOWED and __MAC_OS_X_VERSION_MIN_REQUIRED because the latter header’s comments discuss how it’s appropriate for OS headers, while the former documents its use for user (non-OS) code. TN2064 also recommends <AvailabilityMacros.h>.

Availability.h has the following to say which addresses both of these points:

    It is also possible to use the *_VERSION_MIN_REQUIRED in source code to make one
    source base that can be compiled to target a range of OS versions.  It is best
    to not use the _MAC_* and __IPHONE_* macros for comparisons, but rather their values.
    That is because you might get compiled on an old OS that does not define a later
    OS version macro, and in the C preprocessor undefined values evaluate to zero
    in expresssions, which could cause the #if expression to evaluate in an unexpected
    way.

TN2064 appears to have been last modified in 2003, so it predates the existence of Availability.h. Availability.h was introduced with the iOS SDK in order to support availability macros that are compatible with both OS X and iOS, where the older macros from AvailabilityMacros.h were specific to OS X.

> I never liked the WebKit-specific BUILDING_ON_*/TARGETING_* macros and am happy to see them go.
> 
> My one real gripe with the macros from both <Availability.h> and <AvailabilityMacros.h> is that it’s not terribly obvious which one refers to the SDK (it’s MAX_ALLOWED) and which refers to the deployment target (it’s MIN_REQUIRED). Until you’re familiar with the macros, I think it’s unclear that “allowed” refers to APIs allowed to be called because they’re present in the SDK, and unclear that “required” refers to the runtime requirement. This probably leads to more misuse and abuse than if they had SDK and DT in their names. I guess BUILDING_ON_* and TARGETING_* were a little better in that regard, but they were misused and abused too. SDKs and DTs are probably just too confusing to begin with.

I agree that the terminology used is somewhat confusing. I don't think it's a huge concern though since the minimum required OS version is what the vast majority of the checks within WebKit care about, and that concept is quite easy to understand.

Thanks for taking the time to look this over!

- Mark

> 
> 
> On Tue, Jul 10, 2012 at 7:24 PM, Mark Rowe <mrowe at apple.com> wrote:
> I would like to propose removing the BUILDING_ON and TARGETING family of macros that are used to build code conditionally for different versions of OS X. I propose this in order to address several problems:
> 
> The checks are verbose, and getting worse.
> 
> For instance, in order to write code targeting OS X 10.8 and newer I have to enumerate all other supported OS versions:
> 
> #if !defined(TARGETING_LEOPARD) && !defined(TARGETING_SNOW_LEOPARD) && !defined(TARGETING_LION)
> …
> #endif
> 
> This problem has become worse over time as the number of supported OS X versions in the WebKit code base has increased.
> 
> The nature of the version checks are often not obvious at first glance.
> 
> In order to understand the checks you have to first remember the marketing names of the various OS X releases. You must then reason about the conditional logic of the check itself, which will often contains multiple negated clauses.
> 
> Almost all current uses are incorrect in the context of SDKs.
> 
> The vast majority of the checks in WebKit use the BUILDING_ON macros where the TARGETING macros would be more appropriate. This hasn't cause many problems to date since builds of WebKit on OS X for the most part do not use SDKs. This means that they build with __MAC_OS_X_VERSION_MIN_REQUIRED == __MAC_OS_X_VERSION_MAX_ALLOWED, resulting in the BUILDING_ON and TARGETING macros being equivalent. However, when building with a deployment target of an older OS release than the SDK you're building against, the BUILDING_ON and TARGETING macros will have different behavior. The result is that WebKit fails to build against an SDK when targeting an older OS release.
> 
> My proposed solution to these problems is to remove the BUILDING_ON and TARGETING macros. The vast majority of the BUILDING_ON uses and all of the TARGETING uses would be replaced with tests against __MAC_OS_X_VERSION_MIN_REQUIRED. The small number of uses of BUILDING_ON that are being used correctly would be replaced with tests against __MAC_OS_X_VERSION_MAX_ALLOWED.
> 
> The example above of code targeting OS X 10.8 and newer would become:
> 
> #if __MAC_OS_X_VERSION_MIN_REQUIRED >= 1080
> …
> #endif
> 
> Code that wishes to target only OS X 10.6 and older would become:
> 
> #if __MAC_OS_X_VERSION_MIN_REQUIRED <= 1060
> …
> #endif
> 
> This is much more concise and understandable than the current approach.
> 
> 
> I'm open to feedback on this proposal, but I'd like to move forward with this change in the next day or two if no one objects.
> 
> - Mark
> 
> 
> _______________________________________________
> webkit-dev mailing list
> webkit-dev at lists.webkit.org
> http://lists.webkit.org/mailman/listinfo/webkit-dev
> 
> 
> _______________________________________________
> webkit-dev mailing list
> webkit-dev at lists.webkit.org
> http://lists.webkit.org/mailman/listinfo/webkit-dev

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-dev/attachments/20120711/c892eca8/attachment-0001.html>