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]
:
: Nathan Froyd [:froydnj][high latency until 6 March]
Mentors:
Depends on:
Blocks: 698190
  Show dependency treegraph
 
Reported: 2011-10-18 05:29 PDT by Jonathan Watt [:jwatt]
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]
no flags Details | Diff | Splinter Review
patch (44.10 KB, patch)
2011-10-18 10:59 PDT, Jonathan Watt [:jwatt]
no flags Details | Diff | Splinter Review
patch with Try failures fixed (41.53 KB, patch)
2011-10-25 17:39 PDT, Jonathan Watt [:jwatt]
benjamin: review+
Details | Diff | Splinter Review

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

Thoughts?
Comment 2 User image Jonathan Watt [:jwatt] 2011-10-18 05:32:13 PDT
abort should read "NS_CLAMP min greater than max", or something like that
Comment 3 User image Jonathan Watt [:jwatt] 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 User image Jonathan Watt [:jwatt] 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 User image 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 User image :Ehsan Akhgari 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 User image :Ehsan Akhgari 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 User image Boris Zbarsky [:bz] (still a bit busy) 2011-10-18 10:28:41 PDT
You need to remove the nscore version, right?
Comment 9 User image Jonathan Watt [:jwatt] 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 User image Jonathan Watt [:jwatt] 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 User image 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 User image Jonathan Watt [:jwatt] 2011-10-28 11:40:54 PDT
Pushed https://hg.mozilla.org/integration/mozilla-inbound/rev/21f2d6c4c976
Comment 13 User image Marco Bonardo [::mak] 2011-10-29 01:48:59 PDT
https://hg.mozilla.org/mozilla-central/rev/21f2d6c4c976
Comment 14 User image 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 User image Jonathan Watt [:jwatt] 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.