Bug 144330 – [ES6] Implement String.raw

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Yusuke Suzuki]

[ES6] Implement String.raw

[Yusuke Suzuki]

Before implementing tagged-templates, let's implement String.raw!

[Yusuke Suzuki]

Created attachment 251949 [details]
Prototype

Initial prototype.

[Yusuke Suzuki]

Created attachment 252145 [details]
Patch

[Yusuke Suzuki]

Created attachment 252147 [details]
Patch

[Darin Adler]

Comment on attachment 252147 [details]
Patch

GTK build failure:
DerivedSources/JavaScriptCore/StringConstructor.lut.h:35:83: error: 'stringRaw' was not declared in this scope

[Yusuke Suzuki]

(In reply to comment #5)
> Comment on attachment 252147 [details]
> Patch
> 
> GTK build failure:
> DerivedSources/JavaScriptCore/StringConstructor.lut.h:35:83: error:
> 'stringRaw' was not declared in this scope

I think this is because JSCBuiltin.h is not updated correctly due to CMakeList.txt dependencies.
I'll add blank line to CMakeLists.txt to force update.

[Yusuke Suzuki]

Created attachment 252225 [details]
Patch

[Sam Weinig]

Comment on attachment 252225 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=252225&action=review

> Source/JavaScriptCore/CMakeLists.txt:1215
>      add_dependencies(JavaScriptCore llvmForJSC)
>  endif ()
>  
> +

Seems unrelated.

[Yusuke Suzuki]

Comment on attachment 252225 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=252225&action=review

>> Source/JavaScriptCore/CMakeLists.txt:1215
>> +
> 
> Seems unrelated.

Yes, This is added from the previous patch because GTK's build dependency doesn't flush JSCBuiltins.h correctly. (Due to it, the previous patch fails only in GTK EWS)
https://bugs.webkit.org/show_bug.cgi?id=144330#c6

Adding a blank line to CMakeLists.txt forces CMake to generate new rule files and it refreshes JSCBuiltins.h.
So when landing it, I'll remove this line :D
(And maybe, I need to request full re-build to build bots for GTK ports)

[Yusuke Suzuki]

Could anyone take a look?

[Ryosuke Niwa]

Comment on attachment 252225 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=252225&action=review

> Source/JavaScriptCore/builtins/StringConstructor.js:36
> +    var raw = @Object(rawValue);

I know you're matching the spec text but it's somewhat confusing to have a variable name "raw" inside a function also named "raw".

> Source/JavaScriptCore/builtins/StringConstructor.js:40
> +    var maxSafeInteger = 9007199254740991;

Can we just use 0x1FFFFFFFFFFFFF instead?

> Source/JavaScriptCore/builtins/StringConstructor.js:49
> +    if (numberValue != numberValue) {  // isNaN(numberValue)
> +        lengthValue = 0;
> +    } else if (numberValue === 0 || !@isFinite(numberValue)) {
> +        lengthValue = numberValue;
> +    } else {
> +        lengthValue = (numberValue > 0 ? 1 : -1) * @floor(@abs(numberValue));
> +    }

These line shouldn't have curly braces per our style guideline.

> Source/JavaScriptCore/builtins/StringConstructor.js:51
> +    var literalSegments = lengthValue > 0 ? (lengthValue < maxSafeInteger ? lengthValue : maxSafeInteger) : 0;

It seems like this variable name out to end with nouns such as count and length
since this is really the maximum length we're allowed to process.
Perhaps maximumSegmentCount?

> Source/JavaScriptCore/builtins/StringConstructor.js:57
> +    var nextIndex = 0;

I'm not sure "nextIndex" provides any more semantics than just calling this "i".

> Source/JavaScriptCore/builtins/StringConstructor.js:58
> +    while (true) {

Can we just do:
for (var nextIndex = 0; ; nextIndex++)
instead?

> Source/JavaScriptCore/builtins/StringConstructor.js:68
> +            // |arguments| contain the first argument, |template|.
> +            next = @toString(arguments[nextIndex + 1]);

Instead of adding a comment like this, how about defining two local variables?
var indexInRaw = i;
var indexInArguments = i + 1;

[Yusuke Suzuki]

Comment on attachment 252225 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=252225&action=review

Thank you for your review!

>> Source/JavaScriptCore/builtins/StringConstructor.js:36
>> +    var raw = @Object(rawValue);
> 
> I know you're matching the spec text but it's somewhat confusing to have a variable name "raw" inside a function also named "raw".

Make sense. So replace it with `rawSegments`. (And replace `cooked` to `cookedSegments`)

>> Source/JavaScriptCore/builtins/StringConstructor.js:40
>> +    var maxSafeInteger = 9007199254740991;
> 
> Can we just use 0x1FFFFFFFFFFFFF instead?

Looks nice. Fixed.

>> Source/JavaScriptCore/builtins/StringConstructor.js:49
>> +    }
> 
> These line shouldn't have curly braces per our style guideline.

Oops. Fixed.

>> Source/JavaScriptCore/builtins/StringConstructor.js:51
>> +    var literalSegments = lengthValue > 0 ? (lengthValue < maxSafeInteger ? lengthValue : maxSafeInteger) : 0;
> 
> It seems like this variable name out to end with nouns such as count and length
> since this is really the maximum length we're allowed to process.
> Perhaps maximumSegmentCount?

Make sense. I suggest `segmentCount` because this is the count of the given segment array.

>> Source/JavaScriptCore/builtins/StringConstructor.js:57
>> +    var nextIndex = 0;
> 
> I'm not sure "nextIndex" provides any more semantics than just calling this "i".

OK, just use `i`.

>> Source/JavaScriptCore/builtins/StringConstructor.js:58
>> +    while (true) {
> 
> Can we just do:
> for (var nextIndex = 0; ; nextIndex++)
> instead?

Looks nice.
The current implementation is aligned to the spec. But, we can a little bit clean up the implementation.
I'll rewrite it.

>> Source/JavaScriptCore/builtins/StringConstructor.js:68
>> +            next = @toString(arguments[nextIndex + 1]);
> 
> Instead of adding a comment like this, how about defining two local variables?
> var indexInRaw = i;
> var indexInArguments = i + 1;

Looks nice. I'll use a local variable, `substitutionIndexInArguments`.

[Yusuke Suzuki]

Created attachment 252801 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 252801 [details]
Patch

Clearing flags on attachment: 252801

Committed r184287: <http://trac.webkit.org/changeset/184287>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.