Bug 133785 – Fix build break on EFL port since r169869

• Component: WebKit2
• Status: RESOLVED FIXED

[Gyuyoung Kim]

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
     ^

[Gyuyoung Kim]

Created attachment 232931 [details]
Patch

[Csaba Osztrogonác]

Comment on attachment 232931 [details]
Patch

Clearing flags on attachment: 232931

Committed r169879: <http://trac.webkit.org/changeset/169879>

[Csaba Osztrogonác]

All reviewed patches have been landed.  Closing bug.

[Simon Fraser (smfr)]

Sorry for the breakage.

[Csaba Osztrogonác]

(In reply to comment #4)
> Sorry for the breakage.

Not problem, unfortunately this fix broke Mac, but I'll land the proper fix immediately.

[Csaba Osztrogonác]

Fix landed in https://trac.webkit.org/changeset/169880

[Andy Estes]

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.

[Csaba Osztrogonác]

(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 &&.

[Andy Estes]

> 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.

[Alexey Proskuryakov]

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.

[Csaba Osztrogonác]

(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.

[Csaba Osztrogonác]

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 */

[Andy Estes]

(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.