[webkit-reviews] review denied: [Bug 26978] valueOf called in wrong order in atan2 and date constructors. : [Attachment 68841] Fix for DateConstructor

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Sat Sep 25 22:15:46 PDT 2010


Oliver Hunt <oliver at apple.com> has denied Mark Hahnenberg
<mhahnenb at gmail.com>'s request for review:
Bug 26978: valueOf called in wrong order in atan2 and date constructors.
https://bugs.webkit.org/show_bug.cgi?id=26978

Attachment 68841: Fix for DateConstructor
https://bugs.webkit.org/attachment.cgi?id=68841&action=review

------- Additional Comments from Oliver Hunt <oliver at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=68841&action=review

This needs a changelog as well

> JavaScriptCore/runtime/DateConstructor.cpp:100
> +	   double date_fields[7];
> +	   if (isnan((date_fields[0] = args.at(0).toNumber(exec)))
> +		   || isnan((date_fields[1] = args.at(1).toNumber(exec)))
> +		   || (numArgs >= 3 && isnan((date_fields[2] =
args.at(2).toNumber(exec))))
> +		   || (numArgs >= 4 && isnan((date_fields[3] =
args.at(3).toNumber(exec))))
> +		   || (numArgs >= 5 && isnan((date_fields[4] =
args.at(4).toNumber(exec))))
> +		   || (numArgs >= 6 && isnan((date_fields[5] =
args.at(5).toNumber(exec))))
> +		   || (numArgs >= 7 && isnan((date_fields[6] =
args.at(6).toNumber(exec)))))

I think this be better done as 
double doubleArguments[7] = {args.at(0).toNumber(exec),
args.at(1).toNumber(exec), ...};
then
if (isnan(doubleArguments[0]) || ...)

> JavaScriptCore/runtime/DateConstructor.cpp:172
> +    if (isnan((date_fields[0] = exec->argument(0).toNumber(exec)))
> +	       || isnan((date_fields[1] = exec->argument(1).toNumber(exec)))
> +	       || (n >= 3 && isnan((date_fields[2] =
exec->argument(2).toNumber(exec))))
> +	       || (n >= 4 && isnan((date_fields[3] =
exec->argument(3).toNumber(exec))))
> +	       || (n >= 5 && isnan((date_fields[4] =
exec->argument(4).toNumber(exec))))
> +	       || (n >= 6 && isnan((date_fields[5] =
exec->argument(5).toNumber(exec))))
> +	       || (n >= 7 && isnan((date_fields[6] =
exec->argument(6).toNumber(exec)))))

Similar change to what i recommended above


More information about the webkit-reviews mailing list