[webkit-reviews] review denied: [Bug 30942] Use pointers instead of copies to pass GregorianDateTime objects around : [Attachment 42168] patch

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has denied Geoffrey Garen <ggaren at apple.com>'s
request for review:
Bug 30942: Use pointers instead of copies to pass GregorianDateTime objects
around
https://bugs.webkit.org/show_bug.cgi?id=30942

Attachment 42168: patch
https://bugs.webkit.org/attachment.cgi?id=42168&action=review

------- Additional Comments from David Levin <levin at chromium.org>
This seemed to be intent on preserving the current behavior, but doing it in a
more efficient manner, so I reviewed it with that in mind.

> Index: JavaScriptCore/ChangeLog

Please add a link to the bug.

> +	   (JSC::dateProtoFuncGetYear): Renamed getGregorianDateTime to
gregorianDateTime,
> +	   since it no longer has an out parameter. Uses NULL to indicate
invalid dates.

Since 0 is used in the code instead of NULL, nice to do the same thing here.

> Index: JavaScriptCore/runtime/DateInstance.cpp
> +const GregorianDateTime* DateInstance::gregorianDateTime(ExecState* exec,
bool outputIsUTC) const

> +	   return &m_data->m_cachedGregorianDateTimeUTC;
>      } else {

Since there is now a return at the end of the "if", the "else" should be
removed.


> Index: JavaScriptCore/runtime/DatePrototype.cpp
>  JSValue JSC_HOST_CALL dateProtoFuncToISOString(ExecState* exec, JSObject*,
JSValue thisValue, const ArgList&)
> @@ -454,13 +454,13 @@ JSValue JSC_HOST_CALL dateProtoFuncToISO
> +    const GregorianDateTime* t = thisDateObj->gregorianDateTime(exec,
outputIsUTC);

fwiw, in other places, you changed the name from t to gregorianDateTime.


>  JSValue JSC_HOST_CALL dateProtoFuncSetTime(ExecState* exec, JSObject*,
JSValue thisValue, const ArgList& args)
> @@ -830,16 +830,19 @@ static JSValue setNewValueFromTimeArgs(E
> +    const GregorianDateTime* t = thisDateObj->gregorianDateTime(exec,
inputIsUTC);

fwiw, in other places, you changed the name from t to gregorianDateTime.


> +    if (!t)
> +	   return jsNaN(exec);

This doesn't look right to me and appears to be change in behavior. In other
similar places, it does this:
	 JSValue result = jsNaN(exec);
	 thisDateObj->setInternalValue(result);
	 return result;

It seems like this case isn't covered by current layout tests, so it would be
nice to expand them to hit it.


> @@ -857,26 +860,27 @@ static JSValue setNewValueFromDateArgs(E
> +    if (numArgsToUse == 3 && isnan(milli)) {
> -	   // Based on ECMA 262 15.9.5.40 - .41 (set[UTC]FullYear)
> -	   // the time must be reset to +0 if it is NaN. 

You're removing this comment. Do you think it is no longer valuable?

> +	   JSValue result = jsNumber(exec, 0);
> +	   thisDateObj->setInternalValue(result);
> +	   return result;
>      }

> +    const GregorianDateTime* t = thisDateObj->gregorianDateTime(exec,
inputIsUTC);
> +    if (!t)
> +	   return jsNaN(exec);

This doesn't look right to me and appears to be change in behavior. In other
similar places, it does this:
	 JSValue result = jsNaN(exec);
	 thisDateObj->setInternalValue(result);
	 return result;

It seems like this case isn't covered by current layout tests, so it would be
nice to expand them to hit it.