50385 2010-12-02 07:26:21 -0800 [Qt] Make platform managing of OSAllocator better than r73106 2010-12-02 13:34:09 -0800 1 1 1 Unclassified WebKit Tools / Tests 528+ (Nightly build) All All RESOLVED FIXED Qt, QtTriaged P2 Normal --- 1 ossy webkit-unassigned commit-queue ggaren jturcotte laszlo.gombos paroga yael oldest_to_newest 316134 0 ossy 2010-12-02 07:26:21 -0800 . 316140 1 75375 ossy 2010-12-02 07:35:53 -0800 Created attachment 75375 proposed fix 316142 2 ossy 2010-12-02 07:38:44 -0800 Patrick, Jocelyn, could you verify if the win case is good for all win/wince platform? 316229 3 75375 ggaren 2010-12-02 10:28:20 -0800 Comment on attachment 75375 proposed fix EWS bots are green, so r=me. Thanks! 316233 4 laszlo.gombos 2010-12-02 10:31:11 -0800 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. 316238 5 ggaren 2010-12-02 10:36:50 -0800 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? 316253 6 laszlo.gombos 2010-12-02 11:00:46 -0800 (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. 316257 7 ggaren 2010-12-02 11:03:53 -0800 > 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. 316262 8 paroga 2010-12-02 11:12:37 -0800 (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. 316321 9 75375 commit-queue 2010-12-02 12:09:40 -0800 Comment on attachment 75375 proposed fix Clearing flags on attachment: 75375 Committed r73179: <http://trac.webkit.org/changeset/73179> 316322 10 commit-queue 2010-12-02 12:09:45 -0800 All reviewed patches have been landed. Closing bug. 316397 11 ggaren 2010-12-02 13:34:09 -0800 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. 75375 2010-12-02 07:35:53 -0800 2010-12-02 12:09:40 -0800 proposed fix 1.patch text/plain 2878 ossy ZGlmZiAtLWdpdCBhL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZyBiL0phdmFTY3JpcHRDb3JlL0No YW5nZUxvZwppbmRleCBiM2IwMTM1Li5jN2Q5OTY4IDEwMDY0NAotLS0gYS9KYXZhU2NyaXB0Q29y ZS9DaGFuZ2VMb2cKKysrIGIvSmF2YVNjcmlwdENvcmUvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTUg QEAKKzIwMTAtMTItMDIgIENzYWJhIE9zenRyb2dvbsOhYyAgPG9zc3lAd2Via2l0Lm9yZz4KKwor ICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBbUXRdIE1ha2Ug cGxhdGZvcm0gbWFuYWdpbmcgb2YgT1NBbGxvY2F0b3IgYmV0dGVyIHRoYW4gcjczMTA2CisgICAg ICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD01MDM4NQorCisgICAg ICAgICogd3RmL09TQWxsb2NhdG9yUG9zaXguY3BwOiBSZW1vdmUgcGxhdGZvcm0gc3BlY2lmaWMg Z3VhcmQuCisgICAgICAgICogd3RmL09TQWxsb2NhdG9yU3ltYmlhbi5jcHA6IFJlbW92ZSBwbGF0 Zm9ybSBzcGVjaWZpYyBndWFyZC4KKyAgICAgICAgKiB3dGYvT1NBbGxvY2F0b3JXaW4uY3BwOiBS ZW1vdmUgcGxhdGZvcm0gc3BlY2lmaWMgZ3VhcmQuCisgICAgICAgICogd3RmL3d0Zi5wcmk6IEFk ZCB0aGUgY29ycmVjdCBwbGF0Zm9ybSBzcGVjaWZpYyBzb3VyY2UgZmlsZSBpbnN0ZWFkIG9mIGFs bCBvZiB0aGVtLgorCiAyMDEwLTEyLTAyICBQZXRlciBWYXJnYSAgPHB2YXJnYUBpbmYudS1zemVn ZWQuaHU+CiAKICAgICAgICAgUmV2aWV3ZWQgYnkgR2F2aW4gQmFycmFjbG91Z2guCmRpZmYgLS1n aXQgYS9KYXZhU2NyaXB0Q29yZS93dGYvT1NBbGxvY2F0b3JQb3NpeC5jcHAgYi9KYXZhU2NyaXB0 Q29yZS93dGYvT1NBbGxvY2F0b3JQb3NpeC5jcHAKaW5kZXggZDFkNGFmNS4uMjliODkwNCAxMDA2 NDQKLS0tIGEvSmF2YVNjcmlwdENvcmUvd3RmL09TQWxsb2NhdG9yUG9zaXguY3BwCisrKyBiL0ph dmFTY3JpcHRDb3JlL3d0Zi9PU0FsbG9jYXRvclBvc2l4LmNwcApAQCAtMjYsOCArMjYsNiBAQAog I2luY2x1ZGUgImNvbmZpZy5oIgogI2luY2x1ZGUgIk9TQWxsb2NhdG9yLmgiCiAKLSNpZiBPUyhV TklYKSAmJiAhT1MoU1lNQklBTikKLQogI2luY2x1ZGUgPGVycm5vLmg+CiAjaW5jbHVkZSA8c3lz L21tYW4uaD4KICNpbmNsdWRlIDx3dGYvQXNzZXJ0aW9ucy5oPgpAQCAtODMsNSArODEsMyBAQCB2 b2lkIE9TQWxsb2NhdG9yOjpyZWxlYXNlKHZvaWQqIGFkZHJlc3MsIHNpemVfdCBieXRlcykKIH0K IAogfSAvLyBuYW1lc3BhY2UgV1RGCi0KLSNlbmRpZgpkaWZmIC0tZ2l0IGEvSmF2YVNjcmlwdENv cmUvd3RmL09TQWxsb2NhdG9yU3ltYmlhbi5jcHAgYi9KYXZhU2NyaXB0Q29yZS93dGYvT1NBbGxv Y2F0b3JTeW1iaWFuLmNwcAppbmRleCBjMWNjMDhjLi40NDViNzdhIDEwMDY0NAotLS0gYS9KYXZh U2NyaXB0Q29yZS93dGYvT1NBbGxvY2F0b3JTeW1iaWFuLmNwcAorKysgYi9KYXZhU2NyaXB0Q29y ZS93dGYvT1NBbGxvY2F0b3JTeW1iaWFuLmNwcApAQCAtMjYsOCArMjYsNiBAQAogI2luY2x1ZGUg ImNvbmZpZy5oIgogI2luY2x1ZGUgIk9TQWxsb2NhdG9yLmgiCiAKLSNpZiBPUyhTWU1CSUFOKQot CiAjaW5jbHVkZSA8d3RmL0Zhc3RNYWxsb2MuaD4KIAogbmFtZXNwYWNlIFdURiB7CkBAIC01Niw1 ICs1NCwzIEBAIHZvaWQgT1NBbGxvY2F0b3I6OnJlbGVhc2Uodm9pZCogYWRkcmVzcywgc2l6ZV90 KQogfQogCiB9IC8vIG5hbWVzcGFjZSBXVEYKLQotI2VuZGlmCmRpZmYgLS1naXQgYS9KYXZhU2Ny aXB0Q29yZS93dGYvT1NBbGxvY2F0b3JXaW4uY3BwIGIvSmF2YVNjcmlwdENvcmUvd3RmL09TQWxs b2NhdG9yV2luLmNwcAppbmRleCAzODRmZTEzLi5lYjIxZTM3IDEwMDY0NAotLS0gYS9KYXZhU2Ny aXB0Q29yZS93dGYvT1NBbGxvY2F0b3JXaW4uY3BwCisrKyBiL0phdmFTY3JpcHRDb3JlL3d0Zi9P U0FsbG9jYXRvcldpbi5jcHAKQEAgLTI2LDggKzI2LDYgQEAKICNpbmNsdWRlICJjb25maWcuaCIK ICNpbmNsdWRlICJPU0FsbG9jYXRvci5oIgogCi0jaWYgT1MoV0lORE9XUykKLQogI2luY2x1ZGUg IndpbmRvd3MuaCIKIAogbmFtZXNwYWNlIFdURiB7CkBAIC02MCw1ICs1OCwzIEBAIHZvaWQgT1NB bGxvY2F0b3I6OnJlbGVhc2Uodm9pZCogYWRkcmVzcywgc2l6ZV90IGJ5dGVzKQogfQogCiB9IC8v IG5hbWVzcGFjZSBXVEYKLQotI2VuZGlmCmRpZmYgLS1naXQgYS9KYXZhU2NyaXB0Q29yZS93dGYv d3RmLnByaSBiL0phdmFTY3JpcHRDb3JlL3d0Zi93dGYucHJpCmluZGV4IDBmNzI0MWMuLjZhN2Fj NDEgMTAwNjQ0Ci0tLSBhL0phdmFTY3JpcHRDb3JlL3d0Zi93dGYucHJpCisrKyBiL0phdmFTY3Jp cHRDb3JlL3d0Zi93dGYucHJpCkBAIC0xMyw5ICsxMyw2IEBAIFNPVVJDRVMgKz0gXAogICAgIHd0 Zi9IYXNoVGFibGUuY3BwIFwKICAgICB3dGYvTUQ1LmNwcCBcCiAgICAgd3RmL01haW5UaHJlYWQu Y3BwIFwKLSAgICB3dGYvT1NBbGxvY2F0b3JQb3NpeC5jcHAgXAotICAgIHd0Zi9PU0FsbG9jYXRv clN5bWJpYW4uY3BwIFwKLSAgICB3dGYvT1NBbGxvY2F0b3JXaW4uY3BwIFwKICAgICB3dGYvcXQv TWFpblRocmVhZFF0LmNwcCBcCiAgICAgd3RmL3F0L1N0cmluZ1F0LmNwcCBcCiAgICAgd3RmL3F0 L1RocmVhZGluZ1F0LmNwcCBcCkBAIC00NiwzICs0Myw2IEBAIGNvbnRhaW5zKERFRklORVMsIFVT RV9HU1RSRUFNRVI9MSkgewogICAgIFNPVVJDRVMgKz0gd3RmL1RDU3lzdGVtQWxsb2MuY3BwCiB9 CiAKK3VuaXg6IFNPVVJDRVMgKz0gd3RmL09TQWxsb2NhdG9yUG9zaXguY3BwCitzeW1iaWFuOiBT T1VSQ0VTICs9IHd0Zi9PU0FsbG9jYXRvclN5bWJpYW4uY3BwCit3aW4qfHdpbmNlKjogU09VUkNF UyArPSB3dGYvT1NBbGxvY2F0b3JXaW4uY3BwCg==