786533 - Convert usages of NS_MIN to std::min and remove NS_MIN

Status: RESOLVED FIXED

(Core :: MFBT, defect)

Description

[(no longer active)]

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.

Comment 1

[(no longer active)]

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

Comment 2

[Joshua Cranmer [:jcranmer]]

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.

Comment 3

[(no longer active)]

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

Comment 4

[Venkatesh (:vpit3833)]

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

Comment 5

[(no longer active)]

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.

Comment 6

[Venkatesh (:vpit3833)]

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.

Comment 7

[(no longer active)]

(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.

Comment 8

[(no longer active)]

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

Comment 9

[Venkatesh (:vpit3833)]

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.

Comment 10

[(no longer active)]

(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!

Comment 11

[Venkatesh (:vpit3833)]

Attachment: diff showing NS_MIN and NS_MAX replaced with std::min and std::max respectively

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

Comment 12

[Venkatesh (:vpit3833)]

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 --

Comment 13

[(no longer active)]

(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?

Comment 14

[Venkatesh (:vpit3833)]

(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.

Comment 15

[(no longer active)]

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!

Comment 16

[Venkatesh (:vpit3833)]

Sure, I will find out.

Comment 17

[Mats Palmgren (inactive)]

+++ 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"

Comment 18

[Venkatesh (:vpit3833)]

Attachment: 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.

Comment 19

[(no longer active)]

(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?   :-)

Comment 20

[Venkatesh (:vpit3833)]

(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! :) )

Comment 21

[(no longer active)]

(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.

Comment 22

[Venkatesh (:vpit3833)]

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?

Comment 23

[Mozilla RelEng Bot]

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/[e-mail]-9c48fe2fb6cf

Comment 24

[Mozilla RelEng Bot]

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/[e-mail]-9c48fe2fb6cf

Comment 25

[(no longer active)]

(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.

Comment 26

[Mozilla RelEng Bot]

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/[e-mail]-48802890bd51

Comment 27

[Venkatesh (:vpit3833)]

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?

Comment 28

[Mats Palmgren (inactive)]

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.

Comment 29

[(no longer active)]

Yeah I agree with comment 28.

Comment 30

[Mats Palmgren (inactive)]

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

Comment 31

[Mats Palmgren (inactive)]

Attachment: part 1, (scripted) Replace NS_MIN/NS_MAX with std::min/std::max and #include <algorithm> where needed

Comment 32

[Mats Palmgren (inactive)]

Attachment: part 2, (scripted) Replace NS_MIN/NS_MAX in xpcom/ with XPCOM_MIN/XPCOM_MAX to prevent accidental use

Comment 33

[Mats Palmgren (inactive)]

Attachment: part 3, On OSX, one of the system header files (exception_defines.h) defines 'try' and 'catch' as macros which breaks @try/@catch

Comment 34

[Mats Palmgren (inactive)]

Attachment: part 4, On Windows, windef.h defines 'min' and 'max' as macros which breaks any use of std::min/std::max

Comment 35

[Mats Palmgren (inactive)]

Attachment: part 5, X11/Xlibint.h defines 'min' and 'max' as macros which breaks any use of std::min/std::max

https://tbpl.mozilla.org/?tree=Try&rev=deb3828a1ee6

Comment 36

[Mats Palmgren (inactive)]

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 37

[(no longer active)]

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 38

[(no longer active)]

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 39

[(no longer active)]

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 40

[(no longer active)]

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

Comment 41

[Mats Palmgren (inactive)]

(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...)

Comment 42

[(no longer active)]

(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.

Comment 43

[Mats Palmgren (inactive)]

(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)

Comment 44

[Mats Palmgren (inactive)]

Attachment: c-c: part 1, Remove mailnews/ variation of NS_MIN/NS_MAX for mixed 32/64-bit types

Comment 45

[Mats Palmgren (inactive)]

Attachment: c-c: part 2, (scripted) Replace NS_MIN/NS_MAX with std::min/std::max and #include <algorithm> where needed

(scripted, no need to review in detail)

Comment 46

[Mats Palmgren (inactive)]

Attachment: c-c: part 3, Sprinkle a few -DNOMINMAX and fix uses of std::min with mixed 32/64-bit types

https://tbpl.mozilla.org/?tree=Thunderbird-Try&noignore=1&rev=cc9d595f695d

Comment 47

[(no longer active)]

Mats, are you waiting on me for something here?

Comment 48

[Mats Palmgren (inactive)]

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).

Comment 49

[Mats Palmgren (inactive)]

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

Comment 50

[Mats Palmgren (inactive)]

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.

Comment 51

[Mats Palmgren (inactive)]

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

Comment 52

[Ed Morley [:emorley]]

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 53

[[e-mail]]

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?

Comment 54

[Mats Palmgren (inactive)]

(In reply to [e-mail] 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)’