Last Comment Bug 695303 - Add mozilla::clamped function
: Add mozilla::clamped function
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: XPCOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla10
Assigned To: Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13)
:
Mentors:
Depends on:
Blocks: 698190
  Show dependency treegraph
 
Reported: 2011-10-18 05:29 PDT by Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13)
Modified: 2011-10-29 07:57 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (606 bytes, patch)
2011-10-18 05:30 PDT, Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13)
no flags Details | Diff | Review
patch (44.10 KB, patch)
2011-10-18 10:59 PDT, Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13)
no flags Details | Diff | Review
patch with Try failures fixed (41.53 KB, patch)
2011-10-25 17:39 PDT, Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13)
benjamin: review+
Details | Diff | Review

Description Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13) 2011-10-18 05:29:50 PDT
I'd like to add an NS_CLAMP function.
Comment 1 Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13) 2011-10-18 05:30:24 PDT
Created attachment 567714 [details] [diff] [review]
patch

Thoughts?
Comment 2 Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13) 2011-10-18 05:32:13 PDT
abort should read "NS_CLAMP min greater than max", or something like that
Comment 3 Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13) 2011-10-18 05:52:10 PDT
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] (Away Jun. 27 - Jul. 13) 2011-10-18 05:57:32 PDT
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 [:bsmedberg] 2011-10-18 07:26:19 PDT
I would like it to be not-a-macro and also not UPPERCASE since that is macro decoration. mozilla::clamped?
Comment 6 :Ehsan Akhgari (busy, don't ask for review please) 2011-10-18 08:19:23 PDT
(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 :Ehsan Akhgari (busy, don't ask for review please) 2011-10-18 08:20:03 PDT
(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 [:bz] (Out June 25-July 6) 2011-10-18 10:28:41 PDT
You need to remove the nscore version, right?
Comment 9 Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13) 2011-10-18 10:59:01 PDT
Created attachment 567806 [details] [diff] [review]
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] (Away Jun. 27 - Jul. 13) 2011-10-25 17:39:39 PDT
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 11 Benjamin Smedberg [:bsmedberg] 2011-10-26 10:51:36 PDT
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] (Away Jun. 27 - Jul. 13) 2011-10-28 11:40:54 PDT
Pushed https://hg.mozilla.org/integration/mozilla-inbound/rev/21f2d6c4c976
Comment 13 Marco Bonardo [::mak] 2011-10-29 01:48:59 PDT
https://hg.mozilla.org/mozilla-central/rev/21f2d6c4c976
Comment 14 Ian Neal 2011-10-29 06:37:19 PDT
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] (Away Jun. 27 - Jul. 13) 2011-10-29 07:57:24 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.