[webkit-reviews] review denied: [Bug 129486] Implement Math.hypot : [Attachment 226925] Patch v9

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Mar 17 09:54:56 PDT 2014


Darin Adler <darin at apple.com> has denied Tibor Mészáros
<tmeszaros.u-szeged at partner.samsung.com>'s request for review:
Bug 129486: Implement Math.hypot
https://bugs.webkit.org/show_bug.cgi?id=129486

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

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


review- because of incorrect behavior when Infinity and NaN are combined

> LayoutTests/js/math-expected.txt:94
> +PASS Math.hypot(1, NaN, Infinity) is Infinity

Shouldn’t the expected result here be NaN, not Infinity?

> Source/JavaScriptCore/runtime/MathObject.cpp:113
> +    putDirectNativeFunctionWithoutTransition(vm, globalObject,
Identifier(&vm, "hypot"), 2, mathProtoFuncHypot, NoIntrinsic, DontEnum |
Function);

No test coverage for the number of arguments? We should be testing that.

> Source/JavaScriptCore/runtime/MathObject.cpp:185
> +    Vector<double> args; 

We can get significantly better efficiency, avoiding heap allocation by saying:


    Vector<double, 8> args;

Choosing some number that is larger than the typical number of arguments passed
to this function.

We should also do:

    args.reserveInitialCapacity(argsCount);

> Source/JavaScriptCore/runtime/MathObject.cpp:187
> +	   args.append(exec->uncheckedArgument(i).toNumber(exec));

This can and should be args.uncheckedAppend if we use
args.reserveInitialCapacity above.

> Source/JavaScriptCore/runtime/MathObject.cpp:191
> +	   if (std::isinf(args[i]))
> +	       return
JSValue::encode(jsDoubleNumber(+std::numeric_limits<double>::infinity()));

Please remove this unless the behavior where we expect Infinity even if there
is a NaN is correct. I do not believe it’s correct.

> Source/JavaScriptCore/runtime/MathObject.cpp:198
> +    // Kahan summation algorithm significantly reduces the numerical error
in the total obtained.

I suggest putting this comment before the definitions of sum and compensation;
they seem like part of the loop to me.

> Source/JavaScriptCore/runtime/MathObject.cpp:200
> +    for (unsigned i = 0; i < args.size(); ++i) {
> +	   double scaledArgument = args[i] / max;

A nicer way to write this is:

    for (double argument : args) {
	double scaledArgument = argument / max;

No need for the use of "i" or "size".


More information about the webkit-reviews mailing list