[JSC] Relax line terminators in String to make JSON subset of JS
Created attachment 332515 [details] Patch
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
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
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
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
Created attachment 332525 [details] Patch
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
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
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
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
Created attachment 332532 [details] Patch
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.
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.
Committed r227775: <https://trac.webkit.org/changeset/227775>
<rdar://problem/37018683>
Committed r227780: <https://trac.webkit.org/changeset/227780>
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?
(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)