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

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted 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 98782: Patch
https://bugs.webkit.org/attachment.cgi?id=98782&action=review

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