Exact rooting for strings in jsdate.cpp

RESOLVED FIXED in mozilla21

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Ms2ger, Assigned: Ms2ger)

Tracking

Trunk
mozilla21
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Assignee)

Description

5 years ago
Created attachment 699945 [details] [diff] [review]
Patch v1
Attachment #699945 - Flags: review?(terrence)
(Assignee)

Updated

5 years ago
Blocks: 753203
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.
Attachment #699945 - Flags: review?(terrence) → review+
(In reply to :Ms2ger from comment #0)
> Created attachment 699945 [details] [diff] [review]
> Patch v1

Sweet! Thanks for helping push the rooting along!
(Assignee)

Comment 3

5 years ago
(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.
(Assignee)

Comment 4

5 years ago
Created attachment 700229 [details] [diff] [review]
Part b: Stop passing CallReceiver

This got big enough that I made it a separate patch.
Attachment #700229 - Flags: review?(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.
Attachment #700229 - Flags: review?(terrence) → review+
(Assignee)

Comment 6

5 years ago
https://hg.mozilla.org/mozilla-central/rev/2978e786bb2c
https://hg.mozilla.org/mozilla-central/rev/80d614cfb1f8
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
You need to log in before you can comment on or make changes to this bug.