Bug 50385 - [Qt] Make platform managing of OSAllocator better than r73106
Summary: [Qt] Make platform managing of OSAllocator better than r73106
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords: Qt, QtTriaged
Depends on:
Blocks:
 
Reported: 2010-12-02 07:26 PST by Csaba Osztrogonác
Modified: 2010-12-02 13:34 PST (History)
6 users (show)

See Also:


Attachments
proposed fix (2.81 KB, patch)
2010-12-02 07:35 PST, Csaba Osztrogonác
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Csaba Osztrogonác 2010-12-02 07:26:21 PST
.
Comment 1 Csaba Osztrogonác 2010-12-02 07:35:53 PST
Created attachment 75375 [details]
proposed fix
Comment 2 Csaba Osztrogonác 2010-12-02 07:38:44 PST
Patrick, Jocelyn, could you verify if the win case is good for all win/wince platform?
Comment 3 Geoffrey Garen 2010-12-02 10:28:20 PST
Comment on attachment 75375 [details]
proposed fix

EWS bots are green, so r=me.

Thanks!
Comment 4 Laszlo Gombos 2010-12-02 10:31:11 PST
To me this seems like an opposite direction on where we want to go - this patch moves some logic from cross platform code into Qt specific build system; other ports would have to re-implement these guards now in their own port-specific mechanism.
Comment 5 Geoffrey Garen 2010-12-02 10:36:50 PST
This patch came out of a discussion I started by email, so I'll paste that here for context:

> I ran into some trouble today with the Qt build system. I wanted to use separate files to separate concerns about different OS's like so:
> 
> OSAllocator.h // cross-platform header
> OSAllocatorPosix.cpp // used on POSIX OS's
> OSAllocatorWin.cpp // used on Windows
>
> This fits the convention we use in WebCore, and it avoids cluttering files with #ifdefs and discordant implementations.
>
> It worked fine for most ports, but not for Qt, because Qt uses the same build project for both Linux and Windows.
>
> Is there any way to make portions of Qt's build project conditional based on OS, or to use different projects on different OS's?
Comment 6 Laszlo Gombos 2010-12-02 11:00:46 PST
(In reply to comment #5)
> This patch came out of a discussion I started by email, so I'll paste that here for context:

Thanks Geoffrey.

> > This fits the convention we use in WebCore, and it avoids cluttering files with #ifdefs and discordant implementations.

I think the convention you referring to in WebCore is different as that is relevant for files specific to a single port (e.g. Gtk or Qt). Files in this patch could be shared between ports.

This was discussed in webkit-dev but I can not seems to find the thread anymore. The summary of the discussion is here - http://trac.webkit.org/wiki/Porting%20Macros%20plan.
Comment 7 Geoffrey Garen 2010-12-02 11:03:53 PST
> this patch moves some logic from cross platform code into Qt specific build system;

The old code wasn't cross-platform -- it just put the burden of selecting different code for different platforms on the .cpp file, instead of the platform's build project file.

> other ports would have to re-implement these guards now in their own port-specific mechanism

For minor platform differences, one or two port-specific #ifdefs in a shared file is the most expedient solution, and I agree, it's nice not to have to rewrite the whole file for each port.

However, in this case we have major platform differences -- for example, there is zero shared code between virtual memory allocation on Windows, POSIX, and Symbian.

In such a case, I don't see much benefit to artificially merging platform-specific implementations, and I do see some disadvantages:

* It doesn't match how major ports like Mac and Windows are organized.

* It doesn't match how WebCore is organized.

* It undermines the separation of concerns, and forces you to think about every permutation of ports for every line of port-specific code you write.

* Having to write an "#if OS(WINDOWS)" guard inside OSAllocatorWin.cpp is very surprising. The whole point of OSAllocatorWin.cpp is that you should only build it on Windows. 

* It litters the code with #ifdefs, and often nested #ifdefs, which degrade readability.

* It increases build times.
Comment 8 Patrick R. Gansterer 2010-12-02 11:12:37 PST
(In reply to comment #2)
> Patrick, Jocelyn, could you verify if the win case is good for all win/wince platform?
I didn't compile the new OSAllocatorWin.cpp with WinCE, but I don't think _this_ change will cause any troubles for WinCE.
Comment 9 WebKit Commit Bot 2010-12-02 12:09:40 PST
Comment on attachment 75375 [details]
proposed fix

Clearing flags on attachment: 75375

Committed r73179: <http://trac.webkit.org/changeset/73179>
Comment 10 WebKit Commit Bot 2010-12-02 12:09:45 PST
All reviewed patches have been landed.  Closing bug.
Comment 11 Geoffrey Garen 2010-12-02 13:34:09 PST
I talked this over with some other folks, and the rough consensus was:

- Separating completely different platform implementations into different .cpp files is good;
- Surrounding a file like OSAllocatorWin.cpp with "#if OS(WINDOWS)" is not terribly harmful, so we're willing to do it if it makes some build systems simpler;
- However, building more than one platform implementation file (for example, OSAllocatorWin.cpp and OSAllocatorPosix.cpp) in the same build system is not recommended practice.

Ultimately, this probably argues against the #ifdef changes in r73179. I don't think there's an immediate need to roll them out, however, going forward, I think Laszlo is right that we should keep platform-specific implementation files #ifdef'd.