[webkit-reviews] review granted: [Bug 94364] Add saturation arithmetic support to FractionalLayoutUnit : [Attachment 160176] Patch

[bugzilla-daemon at webkit.org]

Benjamin Poulain <benjamin at webkit.org> has granted Emil A Eklund
<eae at chromium.org>'s request for review:
Bug 94364: Add saturation arithmetic support to FractionalLayoutUnit
https://bugs.webkit.org/show_bug.cgi?id=94364

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

------- Additional Comments from Benjamin Poulain <benjamin at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=160176&action=review


Thank you for fixing the xcodeproj files.

Few little comments bellow, but otherwise this looks correct.

> Source/WTF/ChangeLog:16
> +	   * GNUmakefile.list.am:
> +	   * WTF.gypi:
> +	   * WTF.pro:
> +	   * WTF.vcproj/WTF.vcproj:
> +	   * WTF.xcodeproj/project.pbxproj:

Not the CMakeLists.txt?

> Source/WTF/wtf/Platform.h:835
> +#if !defined(ENABLE_SATURATED_LAYOUT_ARITHMETIC)
> +#define ENABLE_SATURATED_LAYOUT_ARITHMETIC 0
> +#endif
> +
> +#if ENABLE(ENABLE_SATURATED_LAYOUT_ARITHMETIC) &&
!ENABLE(ENABLE_SUBPIXEL_LAYOUT)
> +#error "ENABLE_SATURATED_LAYOUT_ARITHMETIC requires ENABLE_SUBPIXEL_LAYOUT"
> +#endif

Just, please, please, please, if that ends up being unused, please remove the
code.
We have so much dead code around already. :(

> Source/WTF/wtf/SaturatedArithmetic.h:47
> +    // Can only overflow if the signed bit of the two values match. If the
signed
> +    // bit of the result and one of the values differ it did overflow.
> +    if (!((ua ^ ub) >> 31) & (result ^ ua) >> 31)
> +	   result = std::numeric_limits<int>::max() + (ua >> 31);

I have the feeling we can do better with assembly and jump-if-overflow.
But for now that is a good start. :)

> Source/WTF/wtf/SaturatedArithmetic.h:61
> +    if ((ua ^ ub) >> 31 & (result ^ ua) >> 31)
> +	   result = std::numeric_limits<int>::max() + (ua >> 31);

Ditto.

> Source/WebCore/platform/FractionalLayoutUnit.h:67
> -const int intMinForLayoutUnit = -intMaxForLayoutUnit;
> +const int intMinForLayoutUnit = INT_MIN / kFixedPointDenominator;

This looks correct but can't it be tested?

> Source/WebCore/platform/FractionalLayoutUnit.h:138
> +	   REPORT_OVERFLOW(isInBounds(value));

Should't this be?:
REPORT_OVERFLOW(isInBounds(value + epsilon()));
REPORT_OVERFLOW(isInBounds(value - epsilon()));

> Source/WebCore/platform/FractionalLayoutUnit.h:274
> +	   REPORT_OVERFLOW(isInBounds(value));

Not sure how that is even correct but that's the original code so....
Value can be in bounds and still have value * kFixedPointDenominator overflow.

> Source/WebCore/platform/FractionalLayoutUnit.h:286
> +	   REPORT_OVERFLOW(isInBounds(value));

Ditto.