[Webkit-unassigned] [Bug 129486] Implement Math.hypot
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon Mar 17 09:54:59 PDT 2014
https://bugs.webkit.org/show_bug.cgi?id=129486
Darin Adler <darin at apple.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #226925|review?, commit-queue? |review-, commit-queue-
Flag| |
--- Comment #28 from Darin Adler <darin at apple.com> 2014-03-17 09:55:19 PST ---
(From update of attachment 226925)
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".
--
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
More information about the webkit-unassigned
mailing list