Bug 109161 – [WIN] Use NOMINMAX consistently

• Component: New Bugs
• Status: RESOLVED DUPLICATE

[Simon Hausmann]

[WIN] Use NOMINMAX consistently

[Simon Hausmann]

Created attachment 187034 [details]
Patch

[Early Warning System Bot]

Comment on attachment 187034 [details]
Patch

Attachment 187034 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/16431080

[Simon Hausmann]

Created attachment 187039 [details]
Patch

[Patrick R. Gansterer]

Since it's a duplicate of https://bugs.webkit.org/show_bug.cgi?id=73563, please look the the commtents there. I'd define it via buildsystem and remove it from all config.h files

[Jocelyn Turcotte]

(In reply to comment #4)
> Since it's a duplicate of https://bugs.webkit.org/show_bug.cgi?id=73563, please look the the commtents there. I'd define it via buildsystem and remove it from all config.h files

I'm sure Adam had a good reason behind his comment, but since we would have at least 3 build systems to adjust for this I would prefer to keep it in config.h.

I believe that config.h should define what the code expects/needs from a platform, while the build system should take care of smaller details of the configuration and system the binary will run on.

[Jocelyn Turcotte]

Though there is already a NOMINMAX definition in:
Source/JavaScriptCore/JavaScriptCore.vcproj/testapi/testapiCommon.vsprops
Source/JavaScriptCore/JavaScriptCore.vcxproj/testapi/testapiCommon.props
Source/cmake/OptionsWindows.cmake
Tools/DumpRenderTree/win/ImageDiffCommon.vsprops
Tools/DumpRenderTree/win/ImageDiffWinCairoCommon.vsprops

Simon, what do you think?

[Patrick R. Gansterer]

(In reply to comment #5)
> I believe that config.h should define what the code expects/needs from a platform, while the build system should take care of smaller details of the configuration and system the binary will run on

if we have _ONE_ config.h i agree, but since that's not the case i'd prefere the build system option. Otherwise you should move the other defines into the config.h files too.

[Laszlo Gombos]

Can this code be moved to Platform.h ? If it can that I find Platform.h a good place for it.

[Simon Hausmann]

I don't mind trying the build system option then. That would mean removing the #define min from config.h altogether.

[Laszlo Gombos]

(In reply to comment #9)
> I don't mind trying the build system option then. That would mean removing the #define min from config.h altogether.

Unless this is a technical necessity for the Window platform/build system, moving this definition to the build system is not the general direction that the project seems to prefer. I similar (bot not exactly the same) matter - see bug 108191 .

[Patrick R. Gansterer]

(In reply to comment #10)
> (In reply to comment #9)
> > I don't mind trying the build system option then. That would mean removing the #define min from config.h altogether.
> 
> Unless this is a technical necessity for the Window platform/build system, moving this definition to the build system is not the general direction that the project seems to prefer. I similar (bot not exactly the same) matter - see bug 108191 .

The difference is, that in that case you define a variable, which isn't used outside of WebKit, but NOMINMAX is required for a system header, which can be included at any time, where Platform.h can be too late.
I completely agree that the build system isn't the best direction, but unless we have a "commonconfig.h", which gets includes by all other config.h, I still prefere the build system option (for now).

[Brent Fulgham]

Comment on attachment 187039 [details]
Patch

I don't think this patch is needed anymore, now that NOMINMAX has been added to the build property sheets.

[Patrick R. Gansterer]

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