785542 - Convert usages of PR_MIN and PR_MAX to NS_MIN and NS_MAX

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[(no longer active)]

PR_MIN and PR_MAX are ugly macros, we should use NS_MIN and NS_MAX instead of them everywhere outside of NSS and NSPR.

A good way to fix this bug would be to take my script here <https://bugzilla.mozilla.org/attachment.cgi?id=653946>, modify it to replace these macros, run it on the code base, come up with a patch and submit it to the try server.

Comment 1

[Mats Palmgren (inactive)]

I think I would prefer to use std::min/max instead of NS_MIN/MAX.
Is there a reason we can't use those?

Comment 2

[(no longer active)]

(In reply to comment #1)
> I think I would prefer to use std::min/max instead of NS_MIN/MAX.
> Is there a reason we can't use those?

I don't know if all standard library implementations get this right: <http://mxr.mozilla.org/mozilla-central/source/xpcom/string/public/nsAlgorithm.h#41>

Comment 3

[Mats Palmgren (inactive)]

Ah, good point.  I checked the compilers I have access to: g++ 4.2 and
4.6, clang 3.0 and 3.1, VC++ 10 (cl version 16); and they all do the
opposite of what NS_MAX does.  Compatibility like that makes me think
NS_MAX is wrong ;-)

So we should convert NS_MAX(x,y) => std::max(y,x) at least for floats
and doubles (argument evaluation order is undefined in C++ anyway,
although that change would need to be reviewed carefully for any
dependencies we might have).

Comment 4

[Mats Palmgren (inactive)]

(Intel icpc (ICC) 12.1.5 is also compatible with the other compilers)

Comment 5

[(no longer active)]

(In reply to comment #3)
> So we should convert NS_MAX(x,y) => std::max(y,x) at least for floats
> and doubles (argument evaluation order is undefined in C++ anyway,
> although that change would need to be reviewed carefully for any
> dependencies we might have).

Yeah.  I've been meaning to file a bug for both NS_MAX->std::max and NS_MIN->std::min conversion at some point, but I haven't gotten around to do that yet.  For now I'd like to keep the scope of this bug narrow.  :-)

Comment 6

[Joshua Cranmer [:jcranmer]]

(In reply to Mats Palmgren [:mats] from comment #3)
> Ah, good point.  I checked the compilers I have access to: g++ 4.2 and
> 4.6, clang 3.0 and 3.1, VC++ 10 (cl version 16); and they all do the
> opposite of what NS_MAX does.  Compatibility like that makes me think
> NS_MAX is wrong ;-)

This is what the specification says:
template<class T> const T& min(const T& a, const T& b);
template<class T, class Compare>
const T& min(const T& a, const T& b, Compare comp);
1 Requires: Type T is LessThanComparable (Table 18).
2 Returns: The smaller value.
3 Remarks: Returns the first argument when the arguments are equivalent.

[std::max has a similar prose]

So NS_MAX does not obey C++11 rules for std::max.

Comment 7

[(not currently active) Ted Mielczarek]

Would changing the behavior of NS_MAX have any actual consequences in our codebase? If not then let's just do it.

I don't know how we'd test this in configure other than a runtime test, which is problematic for cross-compiles. Per comment 3 it sounds like none of our Tier 1 platforms get this wrong, so I think we should just rely on the standard behavior and not worry about it. Anyone using a weirdo compiler that doesn't obey the standard here can deal with it themselves.

Comment 8

[(no longer active)]

(In reply to Ted Mielczarek [:ted] from comment #7)
> Would changing the behavior of NS_MAX have any actual consequences in our
> codebase? If not then let's just do it.

Matt would probably know best, but that comment was added intentionally, and I don't really understand the code in question.  <http://hg.mozilla.org/mozilla-central/rev/be7cefceceac#l12.12>

> I don't know how we'd test this in configure other than a runtime test,
> which is problematic for cross-compiles. Per comment 3 it sounds like none
> of our Tier 1 platforms get this wrong, so I think we should just rely on
> the standard behavior and not worry about it. Anyone using a weirdo compiler
> that doesn't obey the standard here can deal with it themselves.

Let's see what Matt thinks.  I guess I can live with the PR_MAX(x, y) => std::max(y, x) conversion and hoping that this won't break in the future.

Comment 9

[Mats Palmgren (inactive)]

> So we should convert NS_MAX(x,y) => std::max(y,x) at least for floats
> and doubles

One strategy that might work to avoid argument swapping where it's
not needed is to declare NS_MAX for float and double in nsAlgorithm.h
but not implement them.  Then recompiling layout/generic for example
will result in errors:

layout/generic/nsFrame.cpp:3787: error: undefined reference to 'NS_MAX(float const&, float const&)'
layout/generic/nsGfxScrollFrame.cpp:1474: error: undefined reference to 'NS_MAX(double const&, double const&)'
layout/generic/nsGfxScrollFrame.cpp:3918: error: undefined reference to 'NS_MAX(double const&, double const&)'
layout/generic/nsTextFrameThebes.cpp:6203: error: undefined reference to 'NS_MAX(float const&, float const&)'
layout/generic/nsTextFrameThebes.cpp:4910: error: undefined reference to 'NS_MAX(double const&, double const&)'

After converting these few manually, the remaining NS_MAX can be
converted with a script *without* swapping the arguments.
(AFAIK, we only use NS_MAX for floating point and integral types,
not other types that defines operator> )

Comment 10

[Nathan Froyd [:froydnj]]

So, per comment 7, let's see if it matters.  NS_MAX with std::max behavior:

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

gfx_{min,max} with std:: behavior:

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

Doesn't prove the absence of bugs, but...

Comment 11

[(no longer active)]

(In reply to comment #10)
> So, per comment 7, let's see if it matters.  NS_MAX with std::max behavior:
> 
> https://tbpl.mozilla.org/?tree=Try&rev=020fbfc497ba
> 
> gfx_{min,max} with std:: behavior:
> 
> https://tbpl.mozilla.org/?tree=Try&rev=da825a7aac9c
> 
> Doesn't prove the absence of bugs, but...

Let's just wait for Matt to answer.  I have no confidence in reasoning about code correctness based on runtime tests!  ;-)

FWIW if we do need to swap floating point arguments, I think comment 9 is the best way to approach that problem.

And also, note that the work that needs to happen here does _not_ belong in this bug.  ;-)

Comment 12

[Matt Woodrow (:mattwoodrow)]

The four uses of NS_MAX in gfx/thebes/gfxQuaternion.h rely on the current behaviour, but that is the only place that I am aware of.

If we do swap them, then we should define something locally for that file.

Comment 13

[alihelmy]

Attachment: diff output from hg after running script to switch PRMIN & PRMAX to NSMIN & NSMAX

Comment 14

[alihelmy]

Attachment: Script used to replace PR_MIN with NS_MIN and PR_MAX with NS_MAX

Comment 15

[Mats Palmgren (inactive)]

It seems the script adds a newline at end of file if it's missing -
I'd rather not touch files that doesn't contain PR_MIN/MAX.

Comment 16

[Mats Palmgren (inactive)]

egrep -rl 'PR_MIN|PR_MAX' . | grep -v /nsprpub | grep -v /security/nss

gives:
./hal/gonk/GonkHal.cpp
./media/webrtc/trunk/src/modules/video_capture/main/source/Windows/BasePin.cpp
./layout/base/nsCSSRendering.cpp
./layout/style/nsCSSProps.cpp
./widget/xpwidgets/nsIdleService.cpp
./xpcom/glue/pldhash.cpp
./xpcom/glue/nsTArray.h
./content/media/nsBuiltinDecoderStateMachine.cpp
./netwerk/cache/nsMemoryCacheDevice.cpp
./netwerk/protocol/http/nsHttpConnectionMgr.cpp
./netwerk/protocol/http/nsHttpTransaction.cpp
./netwerk/protocol/websocket/WebSocketChannel.cpp
./dbm/src/hash_buf.c
./dbm/src/hash.c
./dbm/src/h_bigkey.c

and of those, you probably shouldn't touch the last three since they
look like plain old C.

Comment 17

[(no longer active)]

Comment on attachment 656894 [details] [diff] [review]
diff output from hg after running script to switch PRMIN & PRMAX to NSMIN & NSMAX

Review of attachment 656894 [details] [diff] [review]:
-----------------------------------------------------------------

These trailing newline changes are annoying.  I don't know why the script creates these changes.  Can you please get rid of them?  Thanks!

::: dbm/src/h_bigkey.c
@@ +102,5 @@
>  
>  	/* First move the Key */
>  	for (space = FREESPACE(p) - BIGOVERHEAD; key_size;
>  	    space = FREESPACE(p) - BIGOVERHEAD) {
> +		move_bytes = NS_MIN(space, key_size);

As Mats mentioned, we cannot do this replacement in C source code, as NS_MIN is C++ only.  So please revert the changes to the C source files.

::: hal/gonk/GonkHal.cpp
@@ +366,5 @@
>      HAL_LOG(("Error reading from file %s.", filename));
>      return false;
>    }
>  
> +  buf[NS_MIN(numRead, n - 1)] = '\0';

I don't think this will compile, as NS_MIN is a function.

::: layout/style/nsCSSProps.cpp
@@ +105,5 @@
>    eMaxCSSAliasNameSize
>  };
>  
>  struct CSSPropertyAlias {
> +  const char name[NS_MAX(eMaxCSSAliasNameSize, 1)];

This won't work either.

@@ +110,5 @@
>    const nsCSSProperty id;
>    bool enabled;
>  };
>  
> +static CSSPropertyAlias gAliases[NS_MAX(eCSSAliasCount, 1)] = {

Neither will this one.

::: xpcom/glue/nsTArray.h
@@ +1336,5 @@
>    // MOZ_ALIGNED_DECL because GCC is picky about what goes into
>    // __attribute__((aligned(foo))).
>    union {
>      char mAutoBuf[sizeof(nsTArrayHeader) + N * sizeof(elem_type)];
> +    mozilla::AlignedElem<NS_MAX(MOZ_ALIGNOF(Header), MOZ_ALIGNOF(elem_type))> mAlign;

This won't compile either.

Comment 18

[foudfou]

Attachment: first attempt at replacing PR_M{IN,AX} to NS_M{IN,AX}

I was wanting to volunteer for Bug 786533 which is blocked by this bug. So while waiting for [e-mail] to come back (I haven't got any answer after written to him), here is my try.

A few remarks:
* my IDE automatically removes trailing spaces, which I think is a good thing. I can provide a patch without space changes for more comfortable review.
* I sticked to PR_M{IN,AX} on multiple places (see "XXX" comments) because the change to NS_M{IN,AX} would require c++11 support for constexpr.
* there are 2 places (nsHttpConnectionMgr.cpp and nsHttpTransaction.cpp) where I don't understand the code well. I need to check with people from the corresponding module (possibly jduell).
* the patch compiles well on my Linux x86_64 machine, and seemingly on others http://hg.mozilla.org/try/pushloghtml?changeset=41e691a69dd3, http://hg.mozilla.org/try/pushloghtml?changeset=d5e40dfcb27e (with nsAlgorithm include in BasePin.cpp)

The amended script for renaming will follow.

Comment 19

[foudfou]

Attachment: Amended script used to replace PR_MIN with NS_MIN and PR_MAX with NS_MAX

Comment 20

[foudfou]

Attachment: patch without space changes

Here is the same patch with ignored space changes for easier review.

Comment 21

[(no longer active)]

Comment on attachment 663845 [details] [diff] [review]
patch without space changes

Review of attachment 663845 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good generally.  Most of the below comments are nits, except for the one about WebSocketChannel.cpp which is actually a bug!

Please attach an updated version of the patch to the bug when you get a chance.  Thanks!

::: content/media/nsBuiltinDecoderStateMachine.cpp
@@ +2524,4 @@
>    if (mState == DECODER_STATE_SHUTDOWN) {
>      return NS_ERROR_FAILURE;
>    }
> +  aUsecs = NS_MAX(aUsecs, static_cast<int64_t>(0)); // XXX PR_INT64(0) works also

Please remove the XXX comment.

::: layout/style/nsCSSProps.cpp
@@ +98,4 @@
>  #define CSS_PROP_ALIAS(aliasname_, propid_, aliasmethod_, pref_)              \
>    eMaxCSSAliasNameSizeBefore_##aliasmethod_,                                  \
>    eMaxCSSAliasNameSizeWith_##aliasmethod_ =                                   \
> +    PR_MAX(sizeof(#aliasname_), eMaxCSSAliasNameSizeBefore_##aliasmethod_) - 1, // XXX need constexpr

I don't really want us to have these need constexpr comments in our code base, since that is not something that we can get soon, and finding these occurrences is always easy.

::: netwerk/protocol/http/nsHttpConnectionMgr.cpp
@@ +2967,4 @@
>              NS_ABORT_IF_FALSE(0, "Unknown Bad/Red Pipeline Feedback Event");
>          }
>          
> +        const int16_t kPenalty = 25000; // FIXME: name ?

Remove the FIXME comment please.

::: netwerk/protocol/http/nsHttpTransaction.cpp
@@ +1334,5 @@
>          mRestartInProgressVerifier.ToReadBeforeRestart();
>  
>      if (toReadBeforeRestart && *contentRead) {
> +        uint32_t ignore = PR_MIN(toReadBeforeRestart, PR_UINT32_MAX); // XXX does really what's intended ?
> +        ignore = PR_MIN(*contentRead, ignore);                        // XXX MIN == MINUS ?

Please remove these comments, and file a new bug to investigate what this code does/should do.

::: netwerk/protocol/websocket/WebSocketChannel.cpp
@@ +121,4 @@
>      mLastFailure = TimeStamp::Now();
>      // We use a truncated exponential backoff as suggested by RFC 6455,
>      // but multiply by 1.5 instead of 2 to be more gradual.
> +    mNextDelay = NS_MIN(kWSReconnectMaxDelay, mNextDelay * 3/2);

This is not correct, you need to convert kWSReconnectMaxDelay to a double using a static cast and keep multiplying mNextDelay by 1.5.

::: xpcom/glue/nsTArray.h
@@ +1337,4 @@
>    // __attribute__((aligned(foo))).
>    union {
>      char mAutoBuf[sizeof(nsTArrayHeader) + N * sizeof(elem_type)];
> +    mozilla::AlignedElem<PR_MAX(MOZ_ALIGNOF(Header), MOZ_ALIGNOF(elem_type))> mAlign; // XXX c++0x

Please remove this comment too.

::: xpcom/glue/pldhash.cpp
@@ +285,4 @@
>                   "minAlpha < maxAlpha / 2");
>      if (minAlpha >= maxAlpha / 2) {
>          size = PL_DHASH_TABLE_SIZE(table);
> +        minAlpha = (size * maxAlpha - NS_MAX(size / 256, static_cast<uint32_t>(1))) / (2 * size);

You can just use 1u here, instead of the static cast.

Comment 22

[(no longer active)]

Also, please use the instructions here <https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F> when preparing your final patch.  Thanks!

Comment 23

[foudfou]

Attachment: cleaned patch for converting PR_M{IN,AX} to NS_M{IN,AX}

The previous patch was not intended for merging, but rather for discussion (maybe the feedback? flag was not appropriate ?). So here is a (hopefully) clean one:
* with whitespace cleaning
* generalized use of NS_MIN<type1>(arg1, arg2) instead of NS_MIN(arg1, static_cast<type1>(arg2))
* jduell on IRC confirmed the code in nsHttpTransaction.cpp is fine, so I converted in there too
* the revision log states that some occurrences of PR_M{IN,AX} can not be converted
* try looks ok https://tbpl.mozilla.org/?tree=Try&rev=1df0c1f257f9

I don't think including the conversion script in the log revision, as in https://hg.mozilla.org/mozilla-central/log?rev=a16372, is necessary.

Comment 24

[Mats Palmgren (inactive)]

Doing massive whitespace changes together with other changes makes it hard
to do code archeology later, please don't do that.

Comment 25

[foudfou]

Attachment: cleaned without space changes

Mats, do you want to mentor me for removing all trailing spaces and '\r' from the code base, and possibly fix some indentation problems ?

Comment 26

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 664532 [details] [diff] [review]
cleaned without space changes

Review of attachment 664532 [details] [diff] [review]:
-----------------------------------------------------------------

::: media/webrtc/trunk/src/modules/video_capture/main/source/Windows/BasePin.cpp
@@ +215,4 @@
>    if (!mName.empty()) {
>      // Copy at most (max_buffer_size - sizeof(WCHAR)). The -1 is there to
>      // ensure we always have a null terminator.
> +    unsigned int len = NS_MIN((MAX_PIN_NAME-1)*sizeof(WCHAR), (sizeof(WCHAR)*mName.length()));

I think this would be clearer as

NS_MIN(MAX_PIN_NAME - 1, mName.length()) * sizeof(WCHAR)

Comment 27

[Mats Palmgren (inactive)]

(In reply to foudfou from comment #25)
> Mats, do you want to mentor me for removing all trailing spaces and '\r'
> from the code base, and possibly fix some indentation problems ?

I appreciate the offer, but I'm not sure it's worth it.
Fixing whitespace isn't free:
1. it makes it harder read code history, for example:
http://hg.mozilla.org/mozilla-central/annotate/654489f4be25/layout/generic/nsGfxScrollFrame.cpp#l1643
   hovering the links in the left column I can see at a glance the last
   change for each line
2. it can cause merge conflicts (bit rot) when people rebase their
   patches to tip
3. time to make a patch, review it, land it ....

at least for files in layout/ where many parts are relatively old,
I don't think the benefit outweighs the cost.

One change that I think is worth it for layout/ is fixing up
old-style else-branches of the form:
  }
  else {

to what we write in new code:
  } else {

If the commit message says "fix else-branch indentation" and only
"} else {" lines are touched, then I think the damage to history
is acceptable.  And the benefit is greater than fixing whitespace.

Anyway, since you know C++, I hope we have more interesting bugs
for you to fix than whitespace?  ;-)
(and I'll be happy to mentor/review anywhere I can, which is mostly
in layout/ unless the changes are trivial)

Comment 28

[(no longer active)]

Comment on attachment 664532 [details] [diff] [review]
cleaned without space changes

Review of attachment 664532 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, with the comments below addressed.

::: media/webrtc/trunk/src/modules/video_capture/main/source/Windows/BasePin.cpp
@@ +215,4 @@
>    if (!mName.empty()) {
>      // Copy at most (max_buffer_size - sizeof(WCHAR)). The -1 is there to
>      // ensure we always have a null terminator.
> +    unsigned int len = NS_MIN((MAX_PIN_NAME-1)*sizeof(WCHAR), (sizeof(WCHAR)*mName.length()));

Agreed.

::: xpcom/glue/pldhash.cpp
@@ +285,4 @@
>                   "minAlpha < maxAlpha / 2");
>      if (minAlpha >= maxAlpha / 2) {
>          size = PL_DHASH_TABLE_SIZE(table);
> +        minAlpha = (size * maxAlpha - NS_MAX(size / 256, static_cast<uint32_t>(1))) / (2 * size);

Please use 1u as I mentioned before.

Comment 29

[(no longer active)]

(Please attach a new patch which addresses my comments above, and I'll check it in for you.  Thanks!)

Comment 30

[(no longer active)]

foudfou, sorry I did not catch you on IRC today.  To answer your questions, I prefer NS_MIN<uint32_t> as opposed to static_casting the arguments.  NS_MIN is a template function and the compiler tries to guess what type it should instantiate it with given the type of its arguments, but if they have different types, the compiler has no way of knowing which type it should use.  Once you instantiate NS_MIN with a type explicitly, the compiler would convert things like ints to the correct type implicitly.

Comment 31

[foudfou]

Attachment: updated patch, with Ms2ger's suggestion

* incorporated Ms2ger's suggestion
* with '1u' (sorry for the oversight)
* builds ok https://tbpl.mozilla.org/?tree=Try&rev=13b15d71ad43

Comment 32

[(no longer active)]

Thanks a lot for your patch!

https://hg.mozilla.org/integration/mozilla-inbound/rev/afd078179563

Comment 33

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/afd078179563