Closed Bug 830801 Opened 7 years ago Closed 4 years ago

Add -DNOMINMAX to the default OS_CXXFLAGS on Windows

Categories

(Core :: General, enhancement)

All
Windows 7
enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox42 --- fixed

People

(Reporter: mats, Assigned: m_kato)

References

Details

Attachments

(3 files, 2 obsolete files)

Bug 786533 added a few "OS_CXXFLAGS += -DNOMINMAX" to Makefiles[1]
to suppress Windows min/max macros.  It would probably be better to
add that in a central location as the default, since there are
probably just a few places which needs the macros (likely because
#include <algorithm> fails to compile in some configurations; iirc,
it affected building updater.exe and some crashreporter tool)

[1] https://hg.mozilla.org/integration/mozilla-inbound/rev/b59c92bc4059
Blocks: 1188715
Assignee: nobody → m_kato
Comment on attachment 8640910 [details] [diff] [review]
Part 1. Set NOMINMAX define as default

When investigating VS2015, it requires adding more NOMINMAX macro to many moz.build to build Gecko.

So we should add NOMINMAX define as default.
Attachment #8640910 - Flags: review?(mshal)
Attachment #8640910 - Flags: review?(dmajor)
Comment on attachment 8640911 [details] [diff] [review]
Part 2. Remove NOMINMAX define from moz.build

Remove NOMINMAX from moz.build since NOMINMAX is defined as default
Attachment #8640911 - Flags: review?(mshal)
Comment on attachment 8640912 [details] [diff] [review]
Part 3. Remove NOMINMAX define from cpp source except to external code

Remove NOMINMAX define from our cpp code except to 3rd party code
Attachment #8640912 - Flags: review?(dmajor)
Comment on attachment 8640910 [details] [diff] [review]
Part 1. Set NOMINMAX define as default

I'll leave this to mshal as a build peer.
Attachment #8640910 - Flags: review?(dmajor)
Attachment #8640912 - Flags: review?(dmajor) → review?(mshal)
Comment on attachment 8640910 [details] [diff] [review]
Part 1. Set NOMINMAX define as default

I think it might be helpful to include a link to http://support.microsoft.com/kb/143208 in a comment so we know why the define is necessary (it was included in some of the moz.build files).
Attachment #8640910 - Flags: review?(mshal) → review+
Comment on attachment 8640911 [details] [diff] [review]
Part 2. Remove NOMINMAX define from moz.build

It looks like you missed these files:

./toolkit/crashreporter/breakpad-windows-libxul/staticruntime/moz.build:for var in ('UNICODE', 'UNICODE_', 'BREAKPAD_NO_TERMINATE_THREAD', 'NOMINMAX'):

./ipc/chromium/chromium-config.mozbuild:        'NOMINMAX': True,

The rest looks good!
Attachment #8640911 - Flags: review?(mshal) → feedback+
Comment on attachment 8640912 [details] [diff] [review]
Part 3. Remove NOMINMAX define from cpp source except to external code

Do we still need these defines?

./config/msvc-stl-wrapper.template.h:#define NOMINMAX 1
./config/gcc-stl-wrapper.template.h:#define NOMINMAX 1
Attachment #8640912 - Flags: review?(mshal) → review+
Update adding 2 files
Attachment #8640911 - Attachment is obsolete: true
Attachment #8641505 - Flags: review?(mshal)
add config/gcc-stl-wrapper.template.h and msvc-stl-wrapper.template.h
Attachment #8640912 - Attachment is obsolete: true
Attachment #8641521 - Flags: review?(mshal)
Attachment #8641505 - Flags: review?(mshal) → review+
Attachment #8641521 - Flags: review?(mshal) → review+
Duplicate of this bug: 1188715
You need to log in before you can comment on or make changes to this bug.