Bug 63469 – Clean up integer clamping functions in MathExtras.h and support arbitrary numeric types and limits.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Luke Macpherson]

Clean up integer clamping functions in MathExtras.h and support arbitrary numeric types and limits.

[Luke Macpherson]

Created attachment 98750 [details]
Patch

[Darin Adler]

Comment on attachment 98750 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=98750&action=review

> Source/JavaScriptCore/wtf/MathExtras.h:214
> +template<typename T> inline T actualMinimum() {return std::numeric_limits<T>::min();}
> +template<> inline float actualMinimum() {return -std::numeric_limits<float>::max();}
> +template<> inline double actualMinimum() {return -std::numeric_limits<double>::max();}
> +template<typename T> inline T actualMaximum() { return std::numeric_limits<T>::max();}

I think the name “actualMinimum” is a little confusing. It’s like we are having an argument with the designers of numeric_limits in our code. It would be better to use a term that aims for clarity without trying to reuse the same term numeric_limits uses.

If we can’t think of a great name, then I think given how we are using these functions now we should just call them defaultMinimumForClamp and defaultMaximumForClamp for now. Can always rename later.

We normally use spaces inside { } in WebKit code, so the lines above should have some additional spaces added.

> Source/JavaScriptCore/wtf/MathExtras.h:216
> +template<typename To, typename From> inline To clampTo(const From value, const To min = actualMinimum<To>(), const To max = actualMaximum<To>())

The const here have no effect. You should leave them out.

> Source/JavaScriptCore/wtf/MathExtras.h:221
> +    if (static_cast<double>(value) >= static_cast<double>(max))
> +        return max;
> +    if (static_cast<double>(value) <= static_cast<double>(min))
> +        return min;

I don’t think this is a good technique. Converting everything to a double will make this unnecessarily inefficient.

> Source/JavaScriptCore/wtf/MathExtras.h:227
> +    return clampTo<int, double>(value);

You should be able to call this as clampTo<int> here, and the same in all the other functions. I do not think you have to specify double explicitly, and I think it’s better style to not specify it.

> Source/JavaScriptCore/wtf/MathExtras.h:252
> +    return clampTo<int, unsigned>(value);

It seems terrible to convert an unsigned to a double just to clamp it and turn it into an int. I don’t think this is a good change.

[Luke Macpherson]

Created attachment 98771 [details]
Patch

[Luke Macpherson]

I think this addresses the specific issues mentioned in Darin's review.

I've only added a parameter-type overload for doubles, as personally I don't think this is very productive as you'd need to add a function for every "From" type, which defeats the purpose of having templates.

[Darin Adler]

Comment on attachment 98771 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=98771&action=review

Better, but there is a problem here I’d like to see fixed before landing.

> Source/JavaScriptCore/wtf/MathExtras.h:233
> +template<typename To> inline To clampTo(double value, To min = defaultMinimumForClamp<To>(), To max = defaultMaximumForClamp<To>())
> +{
> +    return clampTo<To, double, double>(value, min, max);
> +}

You misunderstood me, I think. This overload should not be needed. The version above with From should work automatically. I tested this with a simple test program and it worked. Please leave this out.

> Source/JavaScriptCore/wtf/MathExtras.h:247
> +    return clampTo<int>(value, 0);

You might find you need 0.0 here instead of 0.

> Source/JavaScriptCore/wtf/MathExtras.h:252
> +    return clampTo<int, float>(value);

You should not need the "float" here.

> Source/JavaScriptCore/wtf/MathExtras.h:257
> +    return clampTo<int, float>(value, 0);

Or here. Although you might need to use 0.0f instead of 0.

> Source/JavaScriptCore/wtf/MathExtras.h:262
> +    return clampTo<int, unsigned>(value, 0);

You should not need the unsigned here. But you may need to use 0U instead of 0.

[Darin Adler]

(In reply to comment #4)
> I've only added a parameter-type overload for doubles, as personally I don't think this is very productive as you'd need to add a function for every "From" type, which defeats the purpose of having templates.

I wasn’t asking for an overload. Hope my review comments make that clear. I can post my test application to demonstrate what I mean about automatically choosing the correct template if it’s still not clear.

[Luke Macpherson]

Oh, got it. You're right I misunderstood - didn't realize the compiler could infer the template type from the parameter type. That does come in handy.

[Luke Macpherson]

Created attachment 98782 [details]
Patch

[Darin Adler]

Comment on attachment 98782 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=98782&action=review

> Source/JavaScriptCore/wtf/MathExtras.h:216
> +template<typename To, typename From, typename Intermediate> inline To clampTo(From value, To min = defaultMinimumForClamp<To>(), To max = defaultMaximumForClamp<To>())

I’m not sure why we need this ability to have an intermediate type different from "From" nor why we need the typecasts on the left sides of the if statements.

Nothing in this patch takes advantage of either of those. Why not just use From and drop the third template argument?

> Source/JavaScriptCore/wtf/MathExtras.h:257
> +    return clampTo<int>(value, 0);

It’s unfortunate that the template function will include an extra check:

    if (value <= 0)
        return 0;

Maybe we can live with this, but it seems unlikely the compiler will be smart enough to optimize it out. Maybe you should leave the function that clamps unsigned to integer alone for now, since the old version is more efficient than the template version.

[Luke Macpherson]

Comment on attachment 98782 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=98782&action=review

>> Source/JavaScriptCore/wtf/MathExtras.h:216
>> +template<typename To, typename From, typename Intermediate> inline To clampTo(From value, To min = defaultMinimumForClamp<To>(), To max = defaultMaximumForClamp<To>())
> 
> I’m not sure why we need this ability to have an intermediate type different from "From" nor why we need the typecasts on the left sides of the if statements.
> 
> Nothing in this patch takes advantage of either of those. Why not just use From and drop the third template argument?

I think there may be some corner cases where it is needed.
1) clampTo<int>((unsigned short)foo, bar, baz); // tricky because the runtime supplied range may not fit inside an unsigned short.
2) clampTo<unsigned>((int)foo); // tricky because UINT_MAX does not fit inside int.
3) clampTo<int>((float)foo); // slightly wrong because float can't hold int without some loss of precision, so the caller may want to use a double here if precision is important.

I anticipate there might be places in the CSS code dealing with shorts and unsigned shorts that are not clamped correctly right now that could use these corner cases.

>> Source/JavaScriptCore/wtf/MathExtras.h:257
>> +    return clampTo<int>(value, 0);
> 
> It’s unfortunate that the template function will include an extra check:
> 
>     if (value <= 0)
>         return 0;
> 
> Maybe we can live with this, but it seems unlikely the compiler will be smart enough to optimize it out. Maybe you should leave the function that clamps unsigned to integer alone for now, since the old version is more efficient than the template version.

Because value is range restricted to values >= 0 by the unsigned type the compiler should know that this is equivalent to if (value == 0) return 0; return value; When I inlined it with a constant value it was optimized out. I wasn't able to find anywhere in the source tree where this function was actually called with unsigned input, so I can't be sure what the compiled output would look like, but then again it's not worth messing up the code to provide a function that is never called either.

[Darin Adler]

(In reply to comment #10)
> I think there may be some corner cases where it is needed.

So you plan to use it directly later? I don’t object to the concept, but I am not sure how someone would use it.

It sounds like it’s going to be easy to accidentally use this incorrectly!

> 1) clampTo<int>((unsigned short)foo, bar, baz); // tricky because the runtime supplied range may not fit inside an unsigned short.

Sure that makes sense.

> 2) clampTo<unsigned>((int)foo); // tricky because UINT_MAX does not fit inside int.

Are we going to just leave this, then, as a trap for a future programmers to fall into? I don’t like functions that are hard to call correctly.

> 3) clampTo<int>((float)foo); // slightly wrong because float can't hold int without some loss of precision, so the caller may want to use a double here if precision is important.

Since we have this function, this is not just a theoretical question. I do understand that a float can’t hold an int. But does it give the wrong answer? What cases fail? Are there really cases where using a double will give us a correct answer?

> I anticipate there might be places in the CSS code dealing with shorts and unsigned shorts that are not clamped correctly right now that could use these corner cases.

The question isn’t whether code “could use” this, but rather what the clear simple rules for using this function are. I want something easy to use correctly and hard to use incorrectly.

> When I inlined it with a constant value it was optimized out.

Good news.

> I wasn't able to find anywhere in the source tree where this function was actually called with unsigned input, so I can't be sure what the compiled output would look like, but then again it's not worth messing up the code to provide a function that is never called either.

If nobody uses the function, then I suggest we remove it.

[Luke Macpherson]

Created attachment 98838 [details]
Patch

[Luke Macpherson]

Turned out there were two unused functions. Removing them both reduced the number of From types to float and double. The only float From type remaining was used for conversion to int. Given that it was losing precision I have made it use double as well, such that the template type only takes double now.

This takes care of all the tricky cases, and means that it can be safely called for all numeric output types. l have specialized long long to cause an error on link if anyone tries to use it.

[Luke Macpherson]

Created attachment 98841 [details]
Patch

[Darin Adler]

Comment on attachment 98841 [details]
Patch

I still don’t understand why we have to convert the float to a double to convert it to an int. I understand that int has more precision than a float does in the range of int, but I don’t understand why we can’t do a correct conversion without first converting to double. At some point I would love to have an answer to that.

[WebKit Review Bot]

Comment on attachment 98841 [details]
Patch

Attachment 98841 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8945611

[WebKit Review Bot]

Comment on attachment 98841 [details]
Patch

Attachment 98841 [details] did not pass cr-mac-ews (chromium):
Output: http://queues.webkit.org/results/8950497

[WebKit Review Bot]

Comment on attachment 98841 [details]
Patch

Attachment 98841 [details] did not pass cr-mac-ews (chromium):
Output: http://queues.webkit.org/results/8945643

[Luke Macpherson]

Created attachment 98933 [details]
Patch

[Luke Macpherson]

Last patch restores original int clampTo(unsigned) function - apparently used in platform code outside the plain webkit build.

[WebKit Review Bot]

Comment on attachment 98933 [details]
Patch

Clearing flags on attachment: 98933

Committed r89943: <http://trac.webkit.org/changeset/89943>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.