[webkit-reviews] review granted: [Bug 230073] Implement round, mod, rem functions for calc : [Attachment 438888] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has granted Nikos Mouchtaris
<nmouchtaris at apple.com>'s request for review:
Bug 230073: Implement round,mod,rem functions for calc
https://bugs.webkit.org/show_bug.cgi?id=230073

Attachment 438888: Patch

https://bugs.webkit.org/attachment.cgi?id=438888&action=review




--- Comment #7 from Simon Fraser (smfr) <simon.fraser at apple.com> ---
Comment on attachment 438888
  --> https://bugs.webkit.org/attachment.cgi?id=438888
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=438888&action=review

> Source/WebCore/ChangeLog:6
> +	   Reviewed by NOBODY (OOPS!).

Add blank line below here.

> Source/WebCore/css/calc/CSSCalcOperationNode.cpp:451
> +	   LOG_WITH_STREAM(Calc, stream << "Failed to create round node because
unable to determine category from " << prettyPrintNodes(values));

round -> step

> Source/WebCore/platform/calc/CalcExpressionOperation.cpp:37
> +    double lowerB = std::floor(a / std::abs(b))*std::abs(b);
> +    double upperB = lowerB + std::abs(b);

Maybe avoid calling std::abs(b) threee times. Also space around *

> Source/WebCore/platform/calc/CalcExpressionOperation.cpp:145
> +	   return std::numeric_limits<double>::quiet_NaN();

Should this ASSERT_NOT_REACHED?

> Source/WebCore/platform/calc/CalcExpressionOperation.cpp:151
> +	   auto upperB = ret.second;
> +	   return upperB;

No need for upperB

> Source/WebCore/platform/calc/CalcExpressionOperation.cpp:157
> +	   auto lowerB = ret.first;

Ditto

> Source/WebCore/platform/calc/CalcExpressionOperation.cpp:163
> +	   auto ret = getNearestMultiples(m_children[0]->evaluate(maxValue),
m_children[1]->evaluate(maxValue));

Put m_children[0]->evaluate(maxValue) etc into local variables?

> Source/WebCore/platform/calc/CalcExpressionOperation.cpp:166
> +	   return std::abs(upperB-m_children[0]->evaluate(maxValue)) <=
std::abs(m_children[1]->evaluate(maxValue)) / 2 ? upperB : lowerB;

space around -