[Webkit-unassigned] [Bug 129486] Implement Math.hypot

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=129486





--- Comment #29 from Tibor Mészáros <tmeszaros.u-szeged at partner.samsung.com>  2014-03-17 11:16:53 PST ---
(In reply to comment #28)
> (From update of attachment 226925 [details])
> 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?

- The standard say: 
If any argument is +∞, the result is +∞.
If any argument is −∞, the result is +∞.
If no argument is +∞ or −∞, and any argument is NaN, the result is NaN.

> 
> > 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.
> 

- added

> > 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.

- Added, and changed code to match with your explanation.

> 
> > 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.

- if it's not there, the NaN will be the result in some cases, so we have to check the inf.

> 
> > 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.
> 

- Moved to above the variables.

> > 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".

- changed

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.