Bug 172081 – [Win] Correct OS(WINDOWS) and PLATFORM(WIN) compiler guards

• Status: RESOLVED
• Resolution: DUPLICATE
• Component: WebCore Misc.

[Daewoong Jang]

Corrects mixed use of OS(WINDOWS) and PLATFORM(WIN) compiler guards. Most of lines within PLATFORM(WIN) guards in JavaScriptCore are actually OS-dependent implementation and can cause runtime errors if the module was built for Windows without PLATFORM(WIN) compiler flag enabled.

[Daewoong Jang]

Created attachment 310047 [details]
Patch

[Daewoong Jang]

Created attachment 310048 [details]
Patch

[Mark Lam]

Comment on attachment 310048 [details]
Patch

r- because ...

According to Platform.h:
...
#elif OS(WINDOWS)
#define WTF_PLATFORM_WIN 1
#endif

Hence, if OS(WINDOWS), then PLATFORM(WIN) is always true.  The 2 are always tied together it seems.  So, why is that not sufficient?

[Daewoong Jang]

(In reply to Mark Lam from comment #3)
> Comment on attachment 310048 [details]
> Patch
> 
> r- because ...
> 
> According to Platform.h:
> ...
> #elif OS(WINDOWS)
> #define WTF_PLATFORM_WIN 1
> #endif
> 
> Hence, if OS(WINDOWS), then PLATFORM(WIN) is always true.  The 2 are always
> tied together it seems.  So, why is that not sufficient?

It is not sufficient when building on Windows with PLATFORM(WIN) turned off(by adding #undef WTF_PLATFORM_WIN to Platform.h) to avoid using Windows platform features.

I agree that this changes are unnecessary as there are only AppleWin and WinCairo ports, but I believe the lines guarded by PLATFORM(WIN) are actually OS-dependent and it would be good to correct them.

So I appreciate if these changes accepted if they are not wrong and harmful.

[Mark Lam]

(In reply to Daewoong Jang from comment #4)
> (In reply to Mark Lam from comment #3)
> > Comment on attachment 310048 [details]
> > Patch
> > 
> > r- because ...
> > 
> > According to Platform.h:
> > ...
> > #elif OS(WINDOWS)
> > #define WTF_PLATFORM_WIN 1
> > #endif
> > 
> > Hence, if OS(WINDOWS), then PLATFORM(WIN) is always true.  The 2 are always
> > tied together it seems.  So, why is that not sufficient?
> 
> It is not sufficient when building on Windows with PLATFORM(WIN) turned
> off(by adding #undef WTF_PLATFORM_WIN to Platform.h) to avoid using Windows
> platform features.
> 
> I agree that this changes are unnecessary as there are only AppleWin and
> WinCairo ports, but I believe the lines guarded by PLATFORM(WIN) are
> actually OS-dependent and it would be good to correct them.
> 
> So I appreciate if these changes accepted if they are not wrong and harmful.

I understand that you're trying to use OS(WINDOWS) without PLATFORM(WIN).  However, I don't understand why one would want to build for Windows and yet not use the Windows platform.  Can you clarify what the difference is, and what use case you are thinking of i.e. why does it make sense to have OS(WINDOWS) without PLATFORM(WIN)?  I may be ignorant about this, but to me, OS(WINDOWS) is synonymous with PLATFORM(WIN), but obviously, you don't think the 2 are the same.  As a result, I'm not quite sure if it is wrong to reclassify the code as belonging to OS(WINDOWS) vs PLATFORM(WIN).  Clarifying the difference may make a stronger case for your change.

I'm also cc'ing some other folks who know Windows better.  They might be able to judge this better.  Thanks.

[Daewoong Jang]

(In reply to Mark Lam from comment #5)
> (In reply to Daewoong Jang from comment #4)
> > (In reply to Mark Lam from comment #3)
> > > Comment on attachment 310048 [details]
> > > Patch
> > > 
> > > r- because ...
> > > 
> > > According to Platform.h:
> > > ...
> > > #elif OS(WINDOWS)
> > > #define WTF_PLATFORM_WIN 1
> > > #endif
> > > 
> > > Hence, if OS(WINDOWS), then PLATFORM(WIN) is always true.  The 2 are always
> > > tied together it seems.  So, why is that not sufficient?
> > 
> > It is not sufficient when building on Windows with PLATFORM(WIN) turned
> > off(by adding #undef WTF_PLATFORM_WIN to Platform.h) to avoid using Windows
> > platform features.
> > 
> > I agree that this changes are unnecessary as there are only AppleWin and
> > WinCairo ports, but I believe the lines guarded by PLATFORM(WIN) are
> > actually OS-dependent and it would be good to correct them.
> > 
> > So I appreciate if these changes accepted if they are not wrong and harmful.
> 
> I understand that you're trying to use OS(WINDOWS) without PLATFORM(WIN). 
> However, I don't understand why one would want to build for Windows and yet
> not use the Windows platform.  Can you clarify what the difference is, and
> what use case you are thinking of i.e. why does it make sense to have
> OS(WINDOWS) without PLATFORM(WIN)?  I may be ignorant about this, but to me,
> OS(WINDOWS) is synonymous with PLATFORM(WIN), but obviously, you don't think
> the 2 are the same.  As a result, I'm not quite sure if it is wrong to
> reclassify the code as belonging to OS(WINDOWS) vs PLATFORM(WIN). 
> Clarifying the difference may make a stronger case for your change.
> 
> I'm also cc'ing some other folks who know Windows better.  They might be
> able to judge this better.  Thanks.

Thank you for your kind reply. Currently I'm working on a WebKit-based project and in the project Windows binaries are being built on some kind of POSIX emulation layer. This is why I remove WTF_PLATFORM_WIN flag. Thank you.

[Daewoong Jang]

*** This bug has been marked as a duplicate of bug 172144 ***