Bug 30942 – Use pointers instead of copies to pass GregorianDateTime objects around

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Geoffrey Garen]

Created attachment 42168 [details]
patch

patch coming.

[David Levin]

Comment on attachment 42168 [details]
patch

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.

[Geoffrey Garen]

> > Index: JavaScriptCore/ChangeLog
> 
> Please add a link to the bug.

Fixed.

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

Fixed.

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

Fixed.

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

"gregorianDateTime" is not an option, since it would conflict with the local name gregorianDateTime. I changed these cases to "other" to match the style of how a copy constructor indicates the object being copied.

> > +    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;

No change in behavior. If gregorianDateTime returns 0, it indicates that the object's internal value is already NaN, so there's no need to set it to NaN. The new code is the same, just less verbose.

> > @@ -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?

The original KJS developers had a habit of commenting most lines of code with the section of the ECMA spec that pertained to them. We've been moving away from that style, since it just clutters the code. 

> > +    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;

Same as above.

I'll post a new patch with the fixes in place.

[Geoffrey Garen]

Created attachment 42235 [details]
patch2

[Darin Adler]

Comment on attachment 42235 [details]
patch2

r=me

Generally we should never be typing "WTF::" anywhere except for the "using WTF::<name>" in the WTF headers. It looks like we’re not following that guideline (created by Maciej) in the code you’re modifying or the code you’re adding.

[Geoffrey Garen]

Committed revision 50359 with some WTF's removed.