695303 - Add mozilla::clamped function

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Jonathan Watt [:jwatt]]

I'd like to add an NS_CLAMP function.

Comment 1

[Jonathan Watt [:jwatt]]

Attachment: patch

Thoughts?

Comment 2

[Jonathan Watt [:jwatt]]

abort should read "NS_CLAMP min greater than max", or something like that

Comment 3

[Jonathan Watt [:jwatt]]

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.

Comment 4

[Jonathan Watt [:jwatt]]

I'm thinking it should also be called NS_CLAMPED to avoid people thinking that it clamps the first argument passed in.

Comment 5

[Benjamin Smedberg]

I would like it to be not-a-macro and also not UPPERCASE since that is macro decoration. mozilla::clamped?

Comment 6

[(no longer active)]

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

Comment 7

[(no longer active)]

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

Comment 8

[Boris Zbarsky [:bzbarsky]]

You need to remove the nscore version, right?

Comment 9

[Jonathan Watt [:jwatt]]

Attachment: patch

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

Comment 10

[Jonathan Watt [:jwatt]]

Attachment: patch with Try failures fixed

I threw this at try, and got a few failures which I've fixed.

Comment 11

[Benjamin Smedberg]

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.

Comment 12

[Jonathan Watt [:jwatt]]

Pushed https://hg.mozilla.org/integration/mozilla-inbound/rev/21f2d6c4c976

Comment 13

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/21f2d6c4c976

Comment 14

[Ian Neal]

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?

Comment 15

[Jonathan Watt [:jwatt]]

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.