[webkit-reviews] review denied: [Bug 86065] FractionalLayoutUnit minor math bugs : [Attachment 141345] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Allan Sandfeld Jensen
<allan.jensen at nokia.com>'s request for review:
Bug 86065: FractionalLayoutUnit minor math bugs
https://bugs.webkit.org/show_bug.cgi?id=86065

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

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=141345&action=review


Some good changes here, but some others that I believe are not helpful.

> Source/WebCore/platform/FractionalLayoutUnit.h:152
> -    static float epsilon() { return 1 / kFixedPointDenominator; }
> +    static float epsilon() { return 1. / kFixedPointDenominator; }

The comment says this should use “float division”, but this will use
double-precision division and then round to float. That’s probably OK because
it happens at compile time.

> Source/WebCore/platform/FractionalLayoutUnit.h:348
> -    long long rawVal = static_cast<long long>(a.rawValue()) * b.rawValue() /
kFixedPointDenominator;
> +    long long rawVal = (static_cast<long long>(a.rawValue()) * b.rawValue())
/ kFixedPointDenominator;

Given my understanding of the C rules for operators and parentheses and order
of operations, I believe adding these parentheses has no effect. Please do not
add them unless you have evidence I am wrong.

> Source/WebCore/platform/FractionalLayoutUnit.h:416
> -    long long rawVal = static_cast<long long>(kFixedPointDenominator) *
a.rawValue() / b.rawValue();
> +    long long rawVal = (static_cast<long long>(kFixedPointDenominator) *
a.rawValue()) / b.rawValue();

Ditto.

> Source/WebCore/platform/FractionalLayoutUnit.h:572
> -    a = a - b;
> +    a = a - b.toFloat();

I don’t think this change adds value.

> Source/WebCore/platform/FractionalLayoutUnit.h:584
> +    a = a.toFloat() * b;

Why does the explicit cast here yield higher precision? It should not. Please
do not make this change unless it’s actually helpful.

> Source/WebCore/platform/FractionalLayoutUnit.h:590
> +    a = a * b.toFloat();

The explicit call to toFloat here should not be needed.