[webkit-reviews] review denied: [Bug 63469] Clean up integer clamping functions in MathExtras.h and support arbitrary numeric types and limits. : [Attachment 98750] 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 98750: Patch
https://bugs.webkit.org/attachment.cgi?id=98750&action=review

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