This bug is about converting everywhere in our tree where we use NS_MIN to use std::min instead (and potentially #include <algorithm> if needed) and remove NS_MIN.

Bug 785542 needs to be fixed before we want to work on this.

I found 29 uses in comm-central as well, including a definition that looks like this: inline uint32_t NS_MIN(uint32_t a, uint64_t b); (blame bug 215450 for that, apparently). If you expand the scope to include NS_MAX, we have another 8 matches. Surprisingly, no PR_MIN/PR_MAX usage, though.

Good point. This needs to happen for comm-central at the same time.

I like to work on this. What needs to be done?

Venkatesh, please first let's wait for bug 785542 to get fixed. Once that happens, you need to go through the code using NS_MIN and NS_MAX in mozilla-central and comm-central and replace those with std::min and std::max, respectively. Since those two functions are defined in the <algorithm> header, you might need to #include that header where needed. Are you familiar with the steps to build Firefox? That would be a good first step.

Alright Ehsan, we wait for it to be fixed. I have built a Firefox a few times and tried xpcshell and mochitest to see how to use them as well.

(In reply to comment #6) > Alright Ehsan, we wait for it to be fixed. I have built a Firefox a few times > and tried xpcshell and mochitest to see how to use them as well. That sounds great. FWIW, that bug is very close to getting fixed, feel free to CC yourself on that bug to get notified when it gets fixed.

Venkatesh, if you're still interested, you can start working on this with the latest changeset on mozilla-central.

Thanks Ehsan, I started replacing the occurrences a few files at a time so that I know where to look when an error occurs. I shouldn't be long before asking you to comment on a diff.

(In reply to comment #9) > Thanks Ehsan, I started replacing the occurrences a few files at a time so > that I know where to look when an error occurs. I shouldn't be long before > asking you to comment on a diff. Sounds great, thanks a lot!

All occurrences of NS_MIN and NS_MAX are replaced except the definitions, and some comments that refer to NS_MIN and NS_MAX.

Ehsan, the attached diff returned some fails on mochitest and one on xpcshell. Please comment on the diff. Here is the xpcshell fail: -- BEGIN PASTE -- TEST-UNEXPECTED-FAIL | /home/venk/mozilla-central/obj-x86_64-unknown-linux-gnu/_tests/xpcshell/netwerk/test/unit/test_bug455311.js | [xpconnect wrapped nsIURI] == [xpconnect wrapped nsIURI] - See following stack: JS frame :: /home/venk/mozilla-central/testing/xpcshell/head.js :: do_throw :: line 451 JS frame :: /home/venk/mozilla-central/testing/xpcshell/head.js :: _do_check_eq :: line 545 JS frame :: /home/venk/mozilla-central/testing/xpcshell/head.js :: do_check_eq :: line 566 JS frame :: /home/venk/mozilla-central/obj-x86_64-unknown-linux-gnu/_tests/xpcshell/netwerk/test/unit/test_bug455311.js :: run_test :: line 122 JS frame :: /home/venk/mozilla-central/testing/xpcshell/head.js :: _execute_test :: line 315 JS frame :: -e :: <TOP_LEVEL> :: line 1 native frame :: <unknown filename> :: <TOP_LEVEL> :: line 0 -- END PASTE --

(In reply to comment #12) > Ehsan, the attached diff returned some fails on mochitest and one on xpcshell. > Please comment on the diff. Do you get these failures every time you run the tests?

(In reply to Ehsan Akhgari [:ehsan] from comment #13) > Do you get these failures every time you run the tests? Yes, I get same fails every time.

Hmm, it's very hard to find the problem by looking at the patch as it's very large. Can you please break it down into smaller pieces and see which one is causing the problem? You can bisect the patch by taking its first half and applying only that and see if you can reproduce the error. If yes, take the applied half and cut it in half itself. Otherwise, the the non-applied half and cut that in half and try to continue doing this until the patch is small enough that we can debug it sanely. Thanks!

Sure, I will find out.

+++ b/content/base/src/nsCrossSiteListenerProxy.cpp @@ -3,6 +3,8 @@ * License, v. 2.0. If a copy of the MPL was not distributed with this * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ +#include <algorithm> + #include "nsCrossSiteListenerProxy.h" #include "nsIChannel.h" #include "nsIHttpChannel.h" I think we want to keep #include X.h first in X.cpp, so please add the new #include after it, like so: #include "nsCrossSiteListenerProxy.h" +#include <algorithm> #include "nsIChannel.h" #include "nsIHttpChannel.h"

Mochitest and xpcshell tests fail on same cases with and without this patch. No new failures are introduced due to this patch. All occurrences of NS_MIN, NS_MAX and their definitions are removed and replaced with std::min, std::max respectively.

(In reply to comment #18) > Created attachment 668969 [details] [diff] [review] > --> https://bugzilla.mozilla.org/attachment.cgi?id=668969&action=edit > full replace of NS_MIN, NS_MAX with std::min and std::max respectively, removed > defnition of NS_MIN, NS_MAX > > Mochitest and xpcshell tests fail on same cases with and without this patch. > No new failures are introduced due to this patch. All occurrences of NS_MIN, > NS_MAX and their definitions are removed and replaced with std::min, std::max > respectively. Can you please link to a try server run for this patch? Also, is it ready for review? :-)

(In reply to Ehsan Akhgari [:ehsan] from comment #19) > Can you please link to a try server run for this patch? Also, is it ready > for review? :-) Sorry, I don't understand, what is link to a try server run for this patch? Ans, yes, please review. (Note to self: Remember to ask! :) )

(In reply to Venkatesh from comment #20) > (In reply to Ehsan Akhgari [:ehsan] from comment #19) > > Can you please link to a try server run for this patch? Also, is it ready > > for review? :-) > > Sorry, I don't understand, what is link to a try server run for this patch? Oh, sorry, please see https://wiki.mozilla.org/ReleaseEngineering/TryServer to get familiar with the try server. I can push your patch to the try server to get build and test results across all platforms. Actually, I tried to do that, but it seems like your patch was based on an older version of mozilla-central and I got a lot of conflicts when trying to apply it. Can you please update your patch to try to get it to apply cleanly on the current tip of mozilla-central? > Ans, yes, please review. (Note to self: Remember to ask! :) ) No problem, I'll review your patch once you attach an updated version of it as I mentioned above.

I got the url: https://tbpl.mozilla.org/?tree=Try&rev=9c48fe2fb6cf after pushing to the try server. Is this the link to the try server you were talking about?

Try run for 9c48fe2fb6cf is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=9c48fe2fb6cf Results (out of 75 total builds): success: 59 warnings: 4 failure: 12 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/venkateshpitta@gmail.com-9c48fe2fb6cf

Try run for 9c48fe2fb6cf is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=9c48fe2fb6cf Results (out of 76 total builds): success: 59 warnings: 4 failure: 13 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/venkateshpitta@gmail.com-9c48fe2fb6cf

(In reply to Venkatesh from comment #22) > I got the url: https://tbpl.mozilla.org/?tree=Try&rev=9c48fe2fb6cf after > pushing to the try server. Is this the link to the try server you were > talking about? Yes, precisely. If you look at the logs for the red build (B) jobs there, you can see the compilation errors.

Try run for 48802890bd51 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=48802890bd51 Results (out of 143 total builds): success: 116 warnings: 12 failure: 15 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/venkateshpitta@gmail.com-48802890bd51

Incremental build was not complaining, but this time I did clean and then make. ~/mozilla-central/xpcom/string/public/* and ~/mozilla-central/xpcom/glue/* don't compile. The error messages always point to algorithm. nsVersionComparator.cpp returns error: #error "STL code can only be used with infallible ::operator new()" when using std::min/max. The other messages, I think, mean this same thing. I found https://bugzilla.mozilla.org/show_bug.cgi?id=779473 when looking around. The error message is same. I don't know how to get the build/test on try server pass on OS X and Windows. What can be done?

I'm guessing the error comes from line 36 here: http://dxr.mozilla.org/mozilla-central/--GENERATED--/dist/stl_wrappers/algorithm.html I don't think we can include <algorithm> in those files... the problem is (I think) some files are compiled twice, once with the condition on line 33 true, and once false, to build different DLLs. We probably need to continue using NS_MAX in those files, but we should #ifdef them in nsAlgorithm.h in that case to prevent accidental use.

Yeah I agree with comment 28.

Venkatesh, are you still working on this? How can we help?

I guess we could also use std::min/max in some xpcom/ files that doesn't trigger the error described in comment 28. But it was simpler to just let xpcom/ continue to use our own template for now (renamed). We can convert those files later if we want.

Comment on attachment 691116 [details] [diff] [review] part 1, (scripted) Replace NS_MIN/NS_MAX with std::min/std::max and #include <algorithm> where needed Review of attachment 691116 [details] [diff] [review]: ----------------------------------------------------------------- r=me if this builds! :-) (I didn't review this very carefully, please let me know if you want me to.)

Comment on attachment 691118 [details] [diff] [review] part 2, (scripted) Replace NS_MIN/NS_MAX in xpcom/ with XPCOM_MIN/XPCOM_MAX to prevent accidental use Review of attachment 691118 [details] [diff] [review]: ----------------------------------------------------------------- This seems like a necessary evil...

Comment on attachment 691119 [details] [diff] [review] part 3, On OSX, one of the system header files (exception_defines.h) defines 'try' and 'catch' as macros which breaks @try/@catch Review of attachment 691119 [details] [diff] [review]: ----------------------------------------------------------------- </sigh>

Comment on attachment 691120 [details] [diff] [review] part 4, On Windows, windef.h defines 'min' and 'max' as macros which breaks any use of std::min/std::max Review of attachment 691120 [details] [diff] [review]: ----------------------------------------------------------------- I guess we can modify the build system to pass that flag unconditionally but I don't feel very strongly (and this won't be the first place in our code where we do this anyways.) r=me

(In reply to Ehsan Akhgari [:ehsan] from comment #37) > r=me if this builds! :-) I had a full green run earlier today: https://tbpl.mozilla.org/?tree=Try&rev=31f918566284 I updated the tree and re-ran the scripted parts just before asking for review - just a quick build+crashtest to verify it's still green: https://tbpl.mozilla.org/?tree=Try&rev=deb3828a1ee6 (looks good so far) > (I didn't review this very carefully, please let me know if you want me to.) You can skip most of it, but please review js/xpconnect/src/qsgen.py and confirm that std::max (and "#include <algorithm>") makes sense for the js/xpconnect/ files. (btw, I'll attached a comm-central patch soon too...)

(In reply to Mats Palmgren [:mats] from comment #41) > > (I didn't review this very carefully, please let me know if you want me to.) > > You can skip most of it, but please review js/xpconnect/src/qsgen.py > and confirm that std::max (and "#include <algorithm>") makes sense for > the js/xpconnect/ files. Yeah they're fine.

(In reply to Ehsan Akhgari [:ehsan] from comment #40) > I guess we can modify the build system to pass that flag unconditionally I actually tried that approach first... but we use the min/max macros in a few places, and unfortunately (at least) one of those hit the #error in our wrapped <algorithm> as described in comment 28. But yeah, I agree that a global "OS_CXXFLAGS += -DNOMINMAX" and working around that in a few places would be better. (I'll file a follow-up bug)

(scripted, no need to review in detail)

Mats, are you waiting on me for something here?

No thanks, I figured it would be better to land it after the next uplift to avoid bit-rotting peoples patches, especially since I was away over the holidays. I'll try to land it towards the end of the week (Jan 11th or so).

Landed the comm-central parts: https://hg.mozilla.org/comm-central/rev/86b275c695fd https://hg.mozilla.org/comm-central/rev/55c8d994eaed https://hg.mozilla.org/comm-central/rev/50104004f24d

https://hg.mozilla.org/integration/mozilla-inbound/rev/037363fa0258 https://hg.mozilla.org/integration/mozilla-inbound/rev/b9d9fc98561b https://hg.mozilla.org/integration/mozilla-inbound/rev/04fb583963dc https://hg.mozilla.org/integration/mozilla-inbound/rev/b59c92bc4059 https://hg.mozilla.org/integration/mozilla-inbound/rev/c26871bd5f5e I'll announce this change in dev.platform as soon as the build looks green.

Filed bug 830801 on (maybe) making -DNOMINMAX the default on Windows.

https://hg.mozilla.org/mozilla-central/rev/037363fa0258 https://hg.mozilla.org/mozilla-central/rev/b9d9fc98561b https://hg.mozilla.org/mozilla-central/rev/04fb583963dc https://hg.mozilla.org/mozilla-central/rev/b59c92bc4059 https://hg.mozilla.org/mozilla-central/rev/c26871bd5f5e https://hg.mozilla.org/mozilla-central/rev/9a7f61363efe

Comment on attachment 692783 [details] [diff] [review] c-c: part 3, Sprinkle a few -DNOMINMAX and fix uses of std::min with mixed 32/64-bit types [Surprised this doesn't give you any 64->32bit truncation warnings] >+ rv = aOutStream->WriteFrom(mInStream, std::min(avail, uint64_t(4096)), &bytesWritten); 4096ULL?

(In reply to neil@parkwaycc.co.uk from comment #53) > [Surprised this doesn't give you any 64->32bit truncation warnings] I don't see any in my Clang/Linux64 build. File a new bug if you see them. > 4096ULL? nsMsgProtocol.cpp:1086:72: error: no matching function for call to ‘min(uint64_t&, long long unsigned int)’