Add mozilla::clamped function

RESOLVED FIXED in mozilla10

Status

()

Core
XPCOM
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: jwatt, Assigned: jwatt)

Tracking

Trunk
mozilla10
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

6 years ago
I'd like to add an NS_CLAMP function.
(Assignee)

Comment 1

6 years ago
Created attachment 567714 [details] [diff] [review]
patch

Thoughts?
Attachment #567714 - Flags: review?(bzbarsky)
(Assignee)

Comment 2

6 years ago
abort should read "NS_CLAMP min greater than max", or something like that
(Assignee)

Comment 3

6 years ago
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.
(Assignee)

Comment 4

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

Comment 5

6 years ago
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?
(Assignee)

Comment 9

6 years ago
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.
Assignee: nobody → jwatt
Attachment #567714 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #567714 - Flags: review?(bzbarsky)
Attachment #567806 - Flags: review?(benjamin)
(Assignee)

Updated

6 years ago
Summary: Add NS_CLAMP function → Add mozilla::clamped function
(Assignee)

Comment 10

6 years ago
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.
Attachment #567806 - Attachment is obsolete: true
Attachment #567806 - Flags: review?(benjamin)
Attachment #569567 - Flags: review?(benjamin)

Comment 11

6 years ago
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.
Attachment #569567 - Flags: review?(benjamin) → review+
(Assignee)

Comment 12

6 years ago
Pushed https://hg.mozilla.org/integration/mozilla-inbound/rev/21f2d6c4c976
https://hg.mozilla.org/mozilla-central/rev/21f2d6c4c976
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla10

Updated

6 years ago
Blocks: 698190

Comment 14

6 years ago
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?
(Assignee)

Comment 15

6 years ago
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.
You need to log in before you can comment on or make changes to this bug.