[webkit-reviews] review denied: [Bug 63469] Clean up integer clamping functions in MathExtras.h and support arbitrary numeric types and limits. : [Attachment 98771] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Luke Macpherson
<macpherson at chromium.org>'s request for review:
Bug 63469: Clean up integer clamping functions in MathExtras.h and support
arbitrary numeric types and limits.
https://bugs.webkit.org/show_bug.cgi?id=63469

Attachment 98771: Patch
https://bugs.webkit.org/attachment.cgi?id=98771&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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.