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.

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

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

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

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

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

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

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.

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

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

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

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

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.

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.

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

I was wanting to volunteer for Bug 786533 which is blocked by this bug. So while waiting for alihelmy@gmail.com 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.

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

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.

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!

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.

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

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

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

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

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.

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

Thanks a lot for your patch! https://hg.mozilla.org/integration/mozilla-inbound/rev/afd078179563