828567 - Exact rooting for strings in jsdate.cpp

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Patch v1

Comment 1

[Terrence Cole [:terrence]]

Comment on attachment 699945 [details] [diff] [review]
Patch v1

Review of attachment 699945 [details] [diff] [review]:
-----------------------------------------------------------------

Nicely done. I'd just like a bit more code motion to help statically ensure that ToLocaleHelper and date_format don't make use of args[0]; details inline.

::: js/src/jsdate.cpp
@@ +767,5 @@
>   *   TZD  = time zone designator (Z or +hh:mm or -hh:mm or missing for local)
>   */
>  
>  static JSBool
> +date_parseISOString(Handle<JSLinearString*> str, double *result, DateTimeInfo *dtInfo)

The ForwardDeclare macros automatically give you HandleLinearString and RootedLinearString. We have reached concensus (sans Waldo) on using the typedefs.

@@ +906,5 @@
>  #undef NEED_NDIGITS
>  }
>  
>  static JSBool
> +date_parseString(Handle<JSLinearString*> str, double *result, DateTimeInfo *dtInfo)

Ditto.

@@ +1191,4 @@
>      if (!str)
> +        return false;
> +
> +    Rooted<JSLinearString*> linearStr(cx, str->ensureLinear(cx));

Ditto.

@@ +2867,5 @@
>      JSAutoByteString fmtbytes(cx, fmt);
>      if (!fmtbytes)
>          return false;
>  
>      return ToLocaleHelper(cx, args, thisObj, fmtbytes.ptr());

Please change ToLocaleHelper and date_format to take MutableHandleValue instead of CallReceiver. They only use the rval so we might as well pass it directly using CallReceiver::rval.

Comment 2

[David Mandelin [:dmandelin]]

(In reply to :Ms2ger from comment #0)
> Created attachment 699945 [details] [diff] [review]
> Patch v1

Sweet! Thanks for helping push the rooting along!

Comment 3

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to Terrence Cole [:terrence] from comment #1)
> Comment on attachment 699945 [details] [diff] [review]
> Patch v1
> 
> Review of attachment 699945 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Nicely done. I'd just like a bit more code motion to help statically ensure
> that ToLocaleHelper and date_format don't make use of args[0]; details
> inline.
> 
> ::: js/src/jsdate.cpp
> @@ +767,5 @@
> >   *   TZD  = time zone designator (Z or +hh:mm or -hh:mm or missing for local)
> >   */
> >  
> >  static JSBool
> > +date_parseISOString(Handle<JSLinearString*> str, double *result, DateTimeInfo *dtInfo)
> 
> The ForwardDeclare macros automatically give you HandleLinearString and
> RootedLinearString. We have reached concensus (sans Waldo) on using the
> typedefs.

That's not actually true; the macro only defined Raw* and Unrooted*. If you tell me where I should add the Handle/Rooted typedefs, I can do that.

Comment 4

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Part b: Stop passing CallReceiver

This got big enough that I made it a separate patch.

Comment 5

[Terrence Cole [:terrence]]

Comment on attachment 700229 [details] [diff] [review]
Part b: Stop passing CallReceiver

Review of attachment 700229 [details] [diff] [review]:
-----------------------------------------------------------------

Yup, that's exactly it.

Comment 6

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/2978e786bb2c
https://hg.mozilla.org/mozilla-central/rev/80d614cfb1f8