Summary: | [JSC] Relax line terminators in String to make JSON subset of JS | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Yusuke Suzuki <ysuzuki> | ||||||||||||||||
Component: | New Bugs | Assignee: | Yusuke Suzuki <ysuzuki> | ||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||
Severity: | Normal | CC: | ews-watchlist, keith_miller, mark.lam, msaboff, rniwa, saam, webkit-bug-importer | ||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||
Attachments: |
|
Description
Yusuke Suzuki
2018-01-29 00:22:04 PST
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> 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) |