Closed Bug 1039774 Opened 10 years ago Closed 10 years ago

Implement ES6 String.raw

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla34

People

(Reporter: gupta.rajagopal, Assigned: gupta.rajagopal)

References

(Blocks 1 open bug, )

Details

(Keywords: dev-doc-complete, feature, Whiteboard: [DocArea=JS])

Attachments

(1 file, 4 obsolete files)

      No description provided.
Blocks: es6
Whiteboard: [DocArea=JS]
Attached patch Patch to implement String.raw v1 (obsolete) — Splinter Review
I have a couple of questions:
1. The spec says "The length property of the raw function is 1." How do I set that?
2. Where should I put tests for ToLength? I was not able to determine the perfect place for them
Attachment #8457735 - Flags: feedback?(jorendorff)
(In reply to guptha from comment #1)
> 1. The spec says "The length property of the raw function is 1." How do I
> set that?

By declaring String_static_raw as 'String_static_raw(cooked, ...substitutionList)'?
Comment on attachment 8457735 [details] [diff] [review]
Patch to implement String.raw v1

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

Stealing review.

See the comment in jsstr.cpp for your first question.

I think it makes sense to extract the ToLength part into its own bug - also, nice to see this!

Where to put tests for it is a good question. Maybe just a .js file under tests/ecma_6? I don't know what a subdirectory for stuff like this should be called. In any case, you can test ToLength by using `var ToLength = getSelfHostedValue('ToLength')` in a shell test.

Also, we absolutely need to inline it in ion.

We have a few builtin methods that should use ToLength but instead use TO_UINT32. Would be nice to update those, too.

Clearing review mostly because I think ToLength should be split out. The rest is largely nits.

::: js/src/builtin/String.js
@@ +185,5 @@
>      return callFunction(std_Function_apply, std_String_fromCharCode, null, elements);
>  }
>  
> +/* ES6 Draft May 22, 2014 21.1.2.4 */
> +function String_static_raw() {

Supply the `callSite` argument and `...substitutions` as proper arguments here instead of using `arguments`.

@@ +186,5 @@
>  }
>  
> +/* ES6 Draft May 22, 2014 21.1.2.4 */
> +function String_static_raw() {
> +    // Step 1. is not relevant

"// Step 1 (implicit)."

@@ +188,5 @@
> +/* ES6 Draft May 22, 2014 21.1.2.4 */
> +function String_static_raw() {
> +    // Step 1. is not relevant
> +    // Step 2.
> +    var numberOfSubstitutions = arguments.length - 1;

This then just becomes `substitutions.length`.

@@ +191,5 @@
> +    // Step 2.
> +    var numberOfSubstitutions = arguments.length - 1;
> +
> +    // Step 3-4.
> +    var cooked = ToObject(arguments[0]);

"ToObject(callSite)"

@@ +210,5 @@
> +    // Step 13.
> +    var nextIndex = 0;
> +
> +    // Step 14.
> +    do {

Change this to a "while {true}" loop. It doesn't really make a functional difference and is slightly less unusual.

@@ +211,5 @@
> +    var nextIndex = 0;
> +
> +    // Step 14.
> +    do {
> +        // Step 14a-d.

Omit the top-level step, and pluralize "Step" here and where applicable below. I.e., this should be
"// Steps a-d."

@@ +212,5 @@
> +
> +    // Step 14.
> +    do {
> +        // Step 14a-d.
> +        var nextSeg = ToString(raw[ToString(nextIndex)]);

Don't use ToString on the index here: the spec requires all (non-symbol) keys to be strings, but in reality engines use numeric keys for indices. All you'd do here is force other parts of the engine to turn this into a numeric key again.

@@ +219,5 @@
> +        resultString = resultString + nextSeg;
> +
> +        // Step 14f.
> +        if (nextIndex + 1 === literalSegments)
> +            break;

Do what the spec says and just return here.

@@ +224,5 @@
> +
> +        // Step 14g-j.
> +        var nextSub;
> +        if (nextIndex < numberOfSubstitutions)
> +            nextSub = ToString(arguments[nextIndex + 1]);

Use the `substitutions` rest arg here instead of `arguments`.

@@ +231,5 @@
> +
> +        // Step 14k.
> +        resultString = resultString + nextSub;
> +
> +        // Step 14l.

Wow, up until just now I liked the new bugzilla font. But this looks almost identical to "141". :(

::: js/src/jsstr.cpp
@@ +4204,5 @@
>  
>  static const JSFunctionSpec string_static_methods[] = {
>      JS_FN("fromCharCode", js::str_fromCharCode, 1, 0),
>      JS_SELF_HOSTED_FN("fromCodePoint", "String_static_fromCodePoint", 0,0),
> +    JS_SELF_HOSTED_FN("raw", "String_static_raw", 0, 0),

Change the first `0` to `1` to set the proper Function#length value.

::: js/src/vm/NumericConversions.h
@@ +296,5 @@
> +/*  ES6 Draft May 22, 2014 7.1.15 ToLength (specialized for doubles). */
> +inline double
> +ToLength(double d)
> +{
> +    d = ToInteger(d);

Hmm, do we have a way to assert that `d` IsInteger? I couldn't quickly find one, but it'd be nice to be able to do that, as the only callsite for this function already does ToInteger.

@@ +300,5 @@
> +    d = ToInteger(d);
> +
> +    if (d <= 0)
> +        return 0;
> +    return d < pow(2, 53) - 1 ? d : pow(2, 53) - 1;

We have Number.MAX_SAFE_INTEGER in the number_constants[] in jsnum.cpp, which is `pow(2, 53) - 1`. Would be nice to extract that as a #define and use it here.
Attachment #8457735 - Flags: feedback?(jorendorff)
Depends on: 1040196
Assignee: nobody → gupta.rajagopal
Status: NEW → ASSIGNED
No longer depends on: 1040196
Depends on: 1040196
Attached patch Patch to implement String.raw v2 (obsolete) — Splinter Review
1. In jsstr.cpp, for JS_SELF_HOSTED_FN I had to use 2 as nargs instead of 1. I believe this is a consequence of the function being static
2. Added tests
3. Removed the ToLength specific parts
Attachment #8457735 - Attachment is obsolete: true
Attachment #8458183 - Flags: review?(till)
Thanks for the review!

(In reply to Till Schneidereit [:till] from comment #3)

> Hmm, do we have a way to assert that `d` IsInteger? I couldn't quickly find
> one, but it'd be nice to be able to do that, as the only callsite for this
> function already does ToInteger.

Well, there is Number_isInteger, but that uses ToInteger - defeats the whole point. Other than that, I can't spot anything either.

> 
> We have Number.MAX_SAFE_INTEGER in the number_constants[] in jsnum.cpp,
> which is `pow(2, 53) - 1`. Would be nice to extract that as a #define and
> use it here.

Where do I put the #define so it is accessible to both vm/NumericConversions.h and jsnum.cpp? I can't seem to find any set of similar constants bunched together.
Comment on attachment 8458183 [details] [diff] [review]
Patch to implement String.raw v2

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

Nice. r=me with the tests fixed.

(In reply to guptha from comment #4)
> 1. In jsstr.cpp, for JS_SELF_HOSTED_FN I had to use 2 as nargs instead of 1.
> I believe this is a consequence of the function being static

No, it's because of the ...rest arg. I should've known this, sorry.

::: js/src/tests/ecma_6/String/raw.js
@@ +13,5 @@
> +cooked.raw = {};
> +SyntaxError("String.raw(cooked)");
> +
> +cooked.raw = {lengt: 0};
> +SyntaxError("String.raw(cooked)");

I don't think these three tests do what you want them to: they just create SyntaxError instances with the passed-in string as the message.

Additionally, these should throw TypeErrors: the syntax is ok, after all. Given that they're caused by failing `ToObject` calls, I'm pretty sure that that's going to happen, so just change these to use assertThrowsInstanceOf.
Attachment #8458183 - Flags: review?(till) → review+
Sorry, I forgot the needinfo. Setting it now.

(In reply to guptha from comment #5)
> Thanks for the review!
> 
> (In reply to Till Schneidereit [:till] from comment #3)
> 
> > Hmm, do we have a way to assert that `d` IsInteger? I couldn't quickly find
> > one, but it'd be nice to be able to do that, as the only callsite for this
> > function already does ToInteger.
> 
> Well, there is Number_isInteger, but that uses ToInteger - defeats the whole
> point. Other than that, I can't spot anything either.
> 
> > 
> > We have Number.MAX_SAFE_INTEGER in the number_constants[] in jsnum.cpp,
> > which is `pow(2, 53) - 1`. Would be nice to extract that as a #define and
> > use it here.
> 
> Where do I put the #define so it is accessible to both
> vm/NumericConversions.h and jsnum.cpp? I can't seem to find any set of
> similar constants bunched together.
Flags: needinfo?(till)
Attached patch Patch to implement String.raw v3 (obsolete) — Splinter Review
I just wanted to confirm that I've correctly modified two of the tests.

1. If raw is {}, ToObject still works. length is undefined, and ToLength returns 0, so the output is ""
2. If raw just has a property called {lengt} instead of length, it is still the same case as in 1
Attachment #8458183 - Attachment is obsolete: true
Attachment #8458858 - Flags: review+
(In reply to guptha from comment #7)
> Sorry, I forgot the needinfo. Setting it now.

I'm sorry - I saw this, but then forgot to reply. :(

> > Well, there is Number_isInteger, but that uses ToInteger - defeats the whole
> > point. Other than that, I can't spot anything either.

You're right - it doesn't make too much sense. I wanted to to a MOZ_ASSERT, so this check wouldn't actually be done in release builds. However, what we'd really want to assert is that ToInteger has been called, not that the value *is* an integer. It could be one without ToInteger having been called, so the assert wouldn't be very useful. Meaning: forget about this comment.

> > 
> > > 
> > > We have Number.MAX_SAFE_INTEGER in the number_constants[] in jsnum.cpp,
> > > which is `pow(2, 53) - 1`. Would be nice to extract that as a #define and
> > > use it here.
> > 
> > Where do I put the #define so it is accessible to both
> > vm/NumericConversions.h and jsnum.cpp? I can't seem to find any set of
> > similar constants bunched together.

Same here: ignore this. I don't really know what would be a good place for this either, and this duplication isn't the end of the world, really. Especially because at both places the magic number is explained by the spec.
Flags: needinfo?(till)
Attached patch Patch to implement String.raw v4 (obsolete) — Splinter Review
Merged.
Attachment #8458858 - Attachment is obsolete: true
Attachment #8463726 - Flags: review+
Try run fixes.

https://tbpl.mozilla.org/?tree=Try&rev=e853ffe398d4
Attachment #8463726 - Attachment is obsolete: true
Attachment #8464830 - Flags: review+
Depends on: 1031397
Keywords: checkin-needed
That try run has a single canceled build. Clearing checkin-needed until we a finished try run is posted.
Flags: needinfo?(gupta.rajagopal)
Keywords: checkin-needed
https://tbpl.mozilla.org/?tree=Try&rev=ec5534acd24a
Flags: needinfo?(gupta.rajagopal)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/8c2a8d5f50bd
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Is this fully covered by automatic tests?
Flags: in-testsuite?
Yes, it is.
Flags: in-testsuite? → in-testsuite+
Blocks: 1102005
Can String.raw be implemented in pure JavaScript? If so, what would that look like?
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: