Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2304923002:
Allow surrogates in string literals. (Closed)
|
Created:
4 years, 3 months ago by Lasse Reichstein Nielsen Modified:
4 years, 2 months ago CC:
reviews_dartlang.org, vm-dev_dartlang.org Target Ref:
refs/heads/master Visibility:
Public. |
DescriptionAllow surrogates in string literals.
Fixes issue #26620
BUG: http://dartbug.com/26620
R=asiva@google.com, brianwilkerson@google.com, floitsch@google.com, hausner@google.com, sigmund@google.com
Committed: https://github.com/dart-lang/sdk/commit/574ae435f3996de9293528393e54f55976a354e6
Committed: https://github.com/dart-lang/sdk/commit/3ba0e9802045c8f0da320a11bea13036a3316b1d
Patch Set 1 #Patch Set 2 : Include analyzer and spec changes. #
Total comments: 7
Patch Set 3 : Add test #Patch Set 4 : Fixed another test #
Total comments: 1
Patch Set 5 : Addressed comment. #Patch Set 6 : Fix bug in dart2js code. Missing return after detecting error. #Messages
Total messages: 19 (6 generated)
lrn@google.com changed reviewers: + floitsch@google.com
Since I did it anyway (it was easy). Still need to update tests before sending it out for review. https://codereview.chromium.org/2304923002/diff/20001/pkg/analyzer/lib/src/ge... File pkg/analyzer/lib/src/generated/parser.dart (left): https://codereview.chromium.org/2304923002/diff/20001/pkg/analyzer/lib/src/ge... pkg/analyzer/lib/src/generated/parser.dart:8118: lexeme.substring(index, currentIndex + 1), +1 here should have been +4. https://codereview.chromium.org/2304923002/diff/20001/pkg/analyzer/lib/src/ge... File pkg/analyzer/lib/src/generated/parser.dart (right): https://codereview.chromium.org/2304923002/diff/20001/pkg/analyzer/lib/src/ge... pkg/analyzer/lib/src/generated/parser.dart:8095: _appendCodePoint(buffer, lexeme, value, index, currentIndex); There was no need to create a new substring when you are also passing the index and currentIndex anyway. The substring is only used for reporting errors, so now it's only created if there is an error. https://codereview.chromium.org/2304923002/diff/20001/pkg/analyzer/lib/src/ge... pkg/analyzer/lib/src/generated/parser.dart:8114: _appendCodePoint( Retained the call to _appendCodePoint here, but it will always do buffer.write(value).
LGTM. Don't forget to mention the issue in the message. https://codereview.chromium.org/2304923002/diff/20001/docs/language/dartLangS... File docs/language/dartLangSpec.tex (right): https://codereview.chromium.org/2304923002/diff/20001/docs/language/dartLangS... docs/language/dartLangSpec.tex:2793: \item $\backslash$u\{$HEX\_DIGIT\_SEQUENCE$\} is the Unicode code point represented by the $HEX\_DIGIT\_SEQUENCE$. It might only be the unicode code unit, since we now allow surrogate pairs. https://codereview.chromium.org/2304923002/diff/20001/pkg/analyzer/lib/src/ge... File pkg/analyzer/lib/src/generated/parser.dart (right): https://codereview.chromium.org/2304923002/diff/20001/pkg/analyzer/lib/src/ge... pkg/analyzer/lib/src/generated/parser.dart:8095: _appendCodePoint(buffer, lexeme, value, index, currentIndex); On 2016/09/15 11:18:44, Lasse Reichstein Nielsen wrote: > There was no need to create a new substring when you are also passing the index > and currentIndex anyway. The substring is only used for reporting errors, so now > it's only created if there is an error. Acknowledged. https://codereview.chromium.org/2304923002/diff/20001/pkg/analyzer/lib/src/ge... pkg/analyzer/lib/src/generated/parser.dart:8114: _appendCodePoint( On 2016/09/15 11:18:44, Lasse Reichstein Nielsen wrote: > Retained the call to _appendCodePoint here, but it will always do > buffer.write(value). > Acknowledged.
https://codereview.chromium.org/2304923002/diff/20001/docs/language/dartLangS... File docs/language/dartLangSpec.tex (right): https://codereview.chromium.org/2304923002/diff/20001/docs/language/dartLangS... docs/language/dartLangSpec.tex:2793: \item $\backslash$u\{$HEX\_DIGIT\_SEQUENCE$\} is the Unicode code point represented by the $HEX\_DIGIT\_SEQUENCE$. The term "code unit" presumes an encoding form (e.g., UTF-16 where code units are 16-bit unsigned integers). Here we are actually talking about the code point itself. The values in the surrogate range are code points but not Unicode scalar values (that's basically the definition of "Unicode scalar value": Code points except high and low surrogates), hence the change. I should put back the "compile-time error" part, though, since six hex digits can specify a value which is not a valid code unit - anything above 0x10FFFF.
Add test
Description was changed from ========== Allow surrogates in string literals. ========== to ========== Allow surrogates in string literals. Fixes issue #26620 BUG: http://dartbug.com/26620 ==========
lrn@google.com changed reviewers: + asiva@google.com, brianwilkerson@google.com, sigmund@google.com
lgtm
lgtm
lrn@google.com changed reviewers: + hausner@google.com
+hausner (or ping siva)
On 2016/09/20 08:08:13, Lasse Reichstein Nielsen wrote: > +hausner (or ping siva) I'll take this as there being no problem with the VM changes. :)
Sorry, I thought Mathias was taking a look at it. LGTM https://codereview.chromium.org/2304923002/diff/60001/tests/language/string_l... File tests/language/string_literals_test.dart (right): https://codereview.chromium.org/2304923002/diff/60001/tests/language/string_l... tests/language/string_literals_test.dart:1: // Copyright (c) 2012, the Dart project authors. Please see the AUTHORS file 2016
My apologies, I overlooked the email with the review request. Thanks for the review, Siva. LGTM 2.
Description was changed from ========== Allow surrogates in string literals. Fixes issue #26620 BUG: http://dartbug.com/26620 ========== to ========== Allow surrogates in string literals. Fixes issue #26620 BUG: http://dartbug.com/26620 R=asiva@google.com, brianwilkerson@google.com, floitsch@google.com, hausner@google.com, sigmund@google.com Committed: https://github.com/dart-lang/sdk/commit/574ae435f3996de9293528393e54f55976a354e6 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) manually as 574ae435f3996de9293528393e54f55976a354e6 (presubmit successful).
Message was sent while issue was closed.
Description was changed from ========== Allow surrogates in string literals. Fixes issue #26620 BUG: http://dartbug.com/26620 R=asiva@google.com, brianwilkerson@google.com, floitsch@google.com, hausner@google.com, sigmund@google.com Committed: https://github.com/dart-lang/sdk/commit/574ae435f3996de9293528393e54f55976a354e6 ========== to ========== Allow surrogates in string literals. Fixes issue #26620 BUG: http://dartbug.com/26620 R=asiva@google.com, brianwilkerson@google.com, floitsch@google.com, hausner@google.com, sigmund@google.com Committed: https://github.com/dart-lang/sdk/commit/574ae435f3996de9293528393e54f55976a354e6 Committed: https://github.com/dart-lang/sdk/commit/3ba0e9802045c8f0da320a11bea13036a3316b1d ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) manually as 3ba0e9802045c8f0da320a11bea13036a3316b1d (presubmit successful). |
