[JSC] Relax line terminators in String to make JSON subset of JS

• Component: New Bugs
• Status: RESOLVED FIXED

[Yusuke Suzuki]

[JSC] Relax line terminators in String to make JSON subset of JS

[Yusuke Suzuki]

Created attachment 332515 [details]
Patch

[EWS Watchlist]

Comment on attachment 332515 [details]
Patch

Attachment 332515 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/6246827

New failing tests:
sputnik/Conformance/07_Lexical_Conventions/7.3_Line_Terminators/S7.3_A2.3.html
sputnik/Conformance/07_Lexical_Conventions/7.3_Line_Terminators/S7.3_A2.4.html

[EWS Watchlist]

Created attachment 332521 [details]
Archive of layout-test-results from ews101 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 332515 [details]
Patch

Attachment 332515 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/6246834

New failing tests:
sputnik/Conformance/07_Lexical_Conventions/7.3_Line_Terminators/S7.3_A2.3.html
sputnik/Conformance/07_Lexical_Conventions/7.3_Line_Terminators/S7.3_A2.4.html

[EWS Watchlist]

Created attachment 332523 [details]
Archive of layout-test-results from ews107 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[Yusuke Suzuki]

Created attachment 332525 [details]
Patch

[EWS Watchlist]

Comment on attachment 332525 [details]
Patch

Attachment 332525 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/6247366

New failing tests:
sputnik/Conformance/07_Lexical_Conventions/7.3_Line_Terminators/S7.3_A2.3.html

[EWS Watchlist]

Created attachment 332530 [details]
Archive of layout-test-results from ews103 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 332525 [details]
Patch

Attachment 332525 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/6247384

New failing tests:
sputnik/Conformance/07_Lexical_Conventions/7.3_Line_Terminators/S7.3_A2.3.html

[EWS Watchlist]

Created attachment 332531 [details]
Archive of layout-test-results from ews104 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[Yusuke Suzuki]

Created attachment 332532 [details]
Patch

[Keith Miller]

Comment on attachment 332532 [details]
Patch

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

r=me with a nit.

> Source/JavaScriptCore/parser/Lexer.cpp:1361
> -            if (atEnd() || isLineTerminator(m_current)) {
> +            if (atEnd() || m_current == '\r' || m_current == '\n') {

Why'd you make this change? Isn't isLineTerminator() always equivalent given the above check (I think LChar/UChar are always unsigned)?

It might also we worth adding:

static_assert(std::is_unsigned<T>::value, "Lexer expects an unsigned character type"); 

somewhere.

[Yusuke Suzuki]

Comment on attachment 332532 [details]
Patch

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

Thanks

>> Source/JavaScriptCore/parser/Lexer.cpp:1361
>> +            if (atEnd() || m_current == '\r' || m_current == '\n') {
> 
> Why'd you make this change? Isn't isLineTerminator() always equivalent given the above check (I think LChar/UChar are always unsigned)?
> 
> It might also we worth adding:
> 
> static_assert(std::is_unsigned<T>::value, "Lexer expects an unsigned character type"); 
> 
> somewhere.

isLineTerminator is different. It returns true for \u2028 and \u2029. The spec change allows \u2028 and \u2029 inside string.
I've added static_assert.

[Yusuke Suzuki]

Committed r227775: <https://trac.webkit.org/changeset/227775>

[Radar WebKit Bug Importer]

<rdar://problem/37018683>

[Yusuke Suzuki]

Committed r227780: <https://trac.webkit.org/changeset/227780>

[Keith Miller]

Comment on attachment 332532 [details]
Patch

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

>>> Source/JavaScriptCore/parser/Lexer.cpp:1361
>>> +            if (atEnd() || m_current == '\r' || m_current == '\n') {
>> 
>> Why'd you make this change? Isn't isLineTerminator() always equivalent given the above check (I think LChar/UChar are always unsigned)?
>> 
>> It might also we worth adding:
>> 
>> static_assert(std::is_unsigned<T>::value, "Lexer expects an unsigned character type"); 
>> 
>> somewhere.
> 
> isLineTerminator is different. It returns true for \u2028 and \u2029. The spec change allows \u2028 and \u2029 inside string.
> I've added static_assert.

Right but the call to isLineTerminator is gated by the if (m_current < 0xE). So \u2028 and \u2029 wouldn't make it do the call. Maybe I'm missing something though?

[Saam Barati]

(In reply to Keith Miller from comment #17)
> Comment on attachment 332532 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=332532&action=review
> 
> >>> Source/JavaScriptCore/parser/Lexer.cpp:1361
> >>> +            if (atEnd() || m_current == '\r' || m_current == '\n') {
> >> 
> >> Why'd you make this change? Isn't isLineTerminator() always equivalent given the above check (I think LChar/UChar are always unsigned)?
> >> 
> >> It might also we worth adding:
> >> 
> >> static_assert(std::is_unsigned<T>::value, "Lexer expects an unsigned character type"); 
> >> 
> >> somewhere.
> > 
> > isLineTerminator is different. It returns true for \u2028 and \u2029. The spec change allows \u2028 and \u2029 inside string.
> > I've added static_assert.
> 
> Right but the call to isLineTerminator is gated by the if (m_current < 0xE).
> So \u2028 and \u2029 wouldn't make it do the call. Maybe I'm missing
> something though?

You're not missing anything. But I agree with Yusuke that it'd be a bit confusing to call that function in this context. (I guess unless we added some kind of assert stating it's definitely not those two unicode values)