Summary: | use NOMINMAX instead of #define min min | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Paul <harris.pc> | ||||||||
Component: | JavaScriptCore | Assignee: | Patrick R. Gansterer <paroga> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | aroben, bfulgham, dglazkov, hausmann, jchaffraix, jedrzej.nowacki, paroga, webkit-ews, webkit.review.bot | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | PC | ||||||||||
OS: | Windows 7 | ||||||||||
Attachments: |
|
Description
Paul
2011-12-01 07:45:09 PST
Would you be willing to submit a patch, as described in <http://www.webkit.org/coding/contributing.html>? Please correct the comment above, too. Created attachment 117817 [details]
Patch
Uploaded patch that adjusts 5 header files to declare NOMINMAX instead of using the trick #define min min This is the more correct way of dealing with windef.h's #define min/max issue on the windows platform. I have not tested it, as I am not set up to build Webkit, sorry. If I need to make more significant changes in the future then I'll go through that process. Also, I'm not sure how to edit previously made comments. Comment on attachment 117817 [details] Patch Attachment 117817 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10729611 New failing tests: svg/custom/linking-uri-01-b.svg I don't see how my patch could've generated an image mismatch in the test suite, I can only assume it was broken before the patch was applied... can that be the case? > I can only assume it was broken before the patch was applied... can that be the case?
It's a flaky test (cf webkit-dev) and is likely not your wrong doing.
Comment on attachment 117817 [details]
Patch
I agree this is better than #define min min!
I think an even better way to do this these days would be to put this in WebKitLibraries/win/tools/vsprops/common.vsprops. All projects inherit settings from there.
Created attachment 212218 [details]
Patch
Comment on attachment 212218 [details] Patch Attachment 212218 [details] did not pass qt-ews (qt): Output: http://webkit-queues.appspot.com/results/2034010 Comment on attachment 212218 [details] Patch Attachment 212218 [details] did not pass qt-wk2-ews (qt-wk2): Output: http://webkit-queues.appspot.com/results/2031013 Created attachment 212221 [details]
Patch
Comment on attachment 212221 [details]
Patch
This looks good. I wish the EWS had been working when you filed the bug, but I don't see any obvious reason for this to fail.
Let's land it and I'll roll it out if there is any build problem.
Comment on attachment 212221 [details] Patch Clearing flags on attachment: 212221 Committed r156302: <http://trac.webkit.org/changeset/156302> All reviewed patches have been landed. Closing bug. *** Bug 109161 has been marked as a duplicate of this bug. *** |