There is build error since rr169869. In file included from /mnt/buildbot/WebKit-BuildSlave/efl-linux-64-release-wk2/build/Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundleAPICast.h:31:0, from /mnt/buildbot/WebKit-BuildSlave/efl-linux-64-release-wk2/build/Source/WebKit2/WebProcess/InjectedBundle/InjectedBundle.cpp:36: /mnt/buildbot/WebKit-BuildSlave/efl-linux-64-release-wk2/build/Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundlePagePrivate.h:96:5: error: "TARGET_OS_IPHONE" is not defined [-Werror=undef] #if TARGET_OS_IPHONE ^
Created attachment 232931 [details] Patch
Comment on attachment 232931 [details] Patch Clearing flags on attachment: 232931 Committed r169879: <http://trac.webkit.org/changeset/169879>
All reviewed patches have been landed. Closing bug.
Sorry for the breakage.
(In reply to comment #4) > Sorry for the breakage. Not problem, unfortunately this fix broke Mac, but I'll land the proper fix immediately.
Fix landed in https://trac.webkit.org/changeset/169880
Why not just do this: #if defined(TARGET_OS_IPHONE) && TARGET_OS_IPHONE On Apple platforms, TARGET_OS_IPHONE is always defined, it's just set to 0 on the Mac. Also, in the .cpp file you can just use PLATFORM(IOS). TARGET_OS_IPHONE is only necessary in API headers.
(In reply to comment #7) > Why not just do this: > #if defined(TARGET_OS_IPHONE) && TARGET_OS_IPHONE It was the first idea, but unfortunately GCC's preprocessor doesn't do short circuit macro evaluation, and the build fails when it tries to evaluate the non defined right side of &&.
> It was the first idea, but unfortunately GCC's preprocessor doesn't do short circuit macro evaluation I didn't know that! Thanks for the explanation.
This fix broke Mac port, fixed in r169900. > It was the first idea, but unfortunately GCC's preprocessor doesn't do short circuit macro evaluation This is very surprising, because we've always used this technique, even before switching to clang. > Fix landed in https://trac.webkit.org/changeset/169880 This patch had [EFL][GTK] in its title despite touching cross-platform files. This confused me for a moment. These bracketed prefixes are intended for bugs and patches that don't touch cross-platform code at all, making them of zero interest to people who don't work on the particular platform. For example, adding code in an #if doesn't qualify, because it could be introducing a design problem that will make refactoring cross-platorfm code harder.
(In reply to comment #10) > This fix broke Mac port, fixed in r169900. Sorry for the breakage, unfortunately we haven't noticed it. +1 for having a buildstep for this and having style checker against c++ style comments in Platform.h ( Bug133802 ) > > It was the first idea, but unfortunately GCC's preprocessor doesn't do short circuit macro evaluation > > This is very surprising, because we've always used this technique, even before switching to clang. After checking it in details, you are true. In this case the short circuit evaluation really works fine with all of my installed GCC version. I confused because of a similar problem, where it doesn't work at all: test.cpp:5:45: error: missing binary operator before token "(" #if defined(__has_include) && __has_include(<stdio.h>) ^ Here only nested ifs help for GCC users. > This patch had [EFL][GTK] in its title despite touching cross-platform files. This confused me for a moment. > > These bracketed prefixes are intended for bugs and patches that don't touch cross-platform code at all, making them of zero interest to people who don't work on the particular platform. For example, adding code in an #if doesn't qualify, because it could be introducing a design problem that will make refactoring cross-platorfm code harder. It's new for me, as far as I remember everybody use [GTK]/[EFL]/[Mac]/[iOS]/[iOS][WK2] title prefix for bugs which touches cross platform files, but surely (or likely) has no effect for other platforms. I remember many [iOS] and [Mac] prefixed bug which touched cross platform code and broke other platform. I'm not against the rule you mentioned, it could help us.
Let's go back to the original problem for a minute. The problem was adding new "#if TARGET_OS_IPHONE" guards which caused undef warning(=error). I don't want to expect using "#if defined(TARGET_OS_IPHONE) && TARGET_OS_IPHONE" always in the future. I'm pretty sure if folks will forget it sometimes and will break the GCC builds accidentally again and again. That's why I added the false definition for TARGET_OS_IPHONE on EFL and GTK. If you don't have objection, I would add back a c style comment to Platform.h like: /* To avoid possible build breakages because of undefined TARGET_OS_IPHONE */
(In reply to comment #12) > Let's go back to the original problem for a minute. The problem was adding > new "#if TARGET_OS_IPHONE" guards which caused undef warning(=error). I > don't want to expect using "#if defined(TARGET_OS_IPHONE) && TARGET_OS_IPHONE" > always in the future. I'm pretty sure if folks will forget it sometimes and > will break the GCC builds accidentally again and again. I don't think this situation will arise very often. We only use TARGET_OS_IPHONE in API headers since those headers are included in translation units that don't use our prefix header. In all other places, we'll use PLATFORM(IOS). Further, only a subset of our API headers are cross-platform (e.g. the Cocoa API headers are only used on Apple platforms), and API headers don't tend to change as often as other files in the project.