I'd like to add an NS_CLAMP function.
Created attachment 567714 [details] [diff] [review]
abort should read "NS_CLAMP min greater than max", or something like that
Oh, it already exists as a macro in nscore.h. Still, it seems like it would be better as an inline function (to only evaluate args once), and better to be in nsAlgorithm.h.
I'm thinking it should also be called NS_CLAMPED to avoid people thinking that it clamps the first argument passed in.
I would like it to be not-a-macro and also not UPPERCASE since that is macro decoration. mozilla::clamped?
(In reply to Jonathan Watt [:jwatt] from comment #2)
> abort should read "NS_CLAMP min greater than max", or something like that
Make it a compile error! Just add a static assert.
(In reply to Ehsan Akhgari [:ehsan] from comment #6)
> (In reply to Jonathan Watt [:jwatt] from comment #2)
> > abort should read "NS_CLAMP min greater than max", or something like that
> Make it a compile error! Just add a static assert.
Well, if it's supposed to be used with constant values for min and max. ;-)
You need to remove the nscore version, right?
Created attachment 567806 [details] [diff] [review]
(In reply to Benjamin Smedberg [:bsmedberg] from comment #5)
> I would like it to be not-a-macro and also not UPPERCASE since that is macro
> decoration. mozilla::clamped?
Sounds good to me.
This patch addresses the comments from other people too, except for the compile time assert thing, since ofter the max/min values are variables.
Created attachment 569567 [details] [diff] [review]
patch with Try failures fixed
I threw this at try, and got a few failures which I've fixed.
Comment on attachment 569567 [details] [diff] [review]
patch with Try failures fixed
Ugh, NS_MIN is a template now? I thought my review comments for that were pretty explicit. I wonder if anyone wants to volunteer to do mozilla::min and mozilla::max and replace the NS_MIN/NS_MAX bits? Anyway r=me for this patch.
Were the two which are NS_MAX(NS_MIN(...)) rather than NS_MIN(NS_MAX(...)) http://mxr.mozilla.org/mozilla-central/search?string=NS_MAX%28NS_MIN not able to be done this way?
There are various permutations of combinations of NS_MAX NS_MIN that I did not "fix". Specifically at this time in m-c there are the lines:
content/media/nsBuiltinDecoderStateMachine.cpp:1762: clock_time = NS_MIN(clock_time, NS_MAX(mVideoFrameEndTime, mAudioEndTime));
layout/base/nsCaret.cpp:812: aBidiLevel = NS_MAX(aBidiLevel, NS_MIN(levelBefore, levelAfter)); // rule c3
layout/base/nsCaret.cpp:813: aBidiLevel = NS_MIN(aBidiLevel, NS_MAX(levelBefore, levelAfter)); // rule c4
layout/base/nsCSSRendering.cpp:3648: gfxFloat suggestedMaxRectHeight = NS_MAX(NS_MIN(ascent, descentLimit), 1.0);
layout/base/nsDisplayList.h:2244: *aWidth = NS_MAX(NS_MIN(xmost1, mXMost) - *aX, 0);
layout/base/nsLayoutUtils.cpp:2511: result = NS_MAX(min, NS_MIN(pref, fill));
layout/generic/nsColumnSetFrame.cpp:380: numColumns = NS_MAX(1, NS_MIN(numColumns, maxColumns));
layout/generic/nsColumnSetFrame.cpp:390: colWidth = NS_MAX(1, NS_MIN(colWidth, availContentWidth));
layout/mathml/nsMathMLChar.cpp:1751: return NS_MAX(bm.width, bm.rightBearing) - NS_MIN(0, bm.leftBearing);
layout/style/nsComputedDOMStyle.cpp:3530: aValue->SetAppUnits(NS_MAX(aMinAppUnits, NS_MIN(val, aMaxAppUnits)));
layout/style/nsComputedDOMStyle.cpp:3544: aValue->SetAppUnits(NS_MAX(aMinAppUnits, NS_MIN(val, aMaxAppUnits)));
layout/style/nsComputedDOMStyle.cpp:3571: aValue->SetAppUnits(NS_MAX(aMinAppUnits, NS_MIN(val, aMaxAppUnits)));
layout/style/nsComputedDOMStyle.cpp:3581: aValue->SetAppUnits(NS_MAX(aMinAppUnits, NS_MIN(val, aMaxAppUnits)));
layout/style/nsRuleNode.cpp:2471: return NS_MIN(scriptLevelSize, NS_MAX(*aUnconstrainedSize, minScriptSize));
(There may be more where the NS_MIN and NS_MAX appear on different lines.)
Some of these (e.g. the line in nsCSSRendering.cpp) sometimes fail the assertion in mozilla::clamped (max <= min). One was left alone for closeness to the verbiage in the CSS spec (the second line in nsColumnSetFrame.cpp). Others were left because using clamped() caused failures on Try and I didn't think converting them was the best use of my time. And some lines I think are new since I wrote the patch.
I don't plan to work on converting any of the above to clamped(), but that shouldn't stop anyone else that wants to.