Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(123)

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 11418115: Fix Unicode issues in dart2js and dart2dart. (Closed)

Created:
8 years, 1 month ago by erikcorry
Modified:
8 years ago
Reviewers:
floitsch, cshapiro
CC:
reviews_dartlang.org
Visibility:
Public.

Description

Fix Unicode issues in dart2js and dart2dart. R=floitsch@google.com BUG=

Patch Set 1 #

Total comments: 12

Patch Set 2 : Small change to test expectations #

Patch Set 3 : Remove accidental test expectation dump #

Patch Set 4 : Remove accidental test expectation dupe #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+251 lines, -196 lines) Patch
M runtime/tests/vm/vm.status View 1 chunk +2 lines, -0 lines 0 comments Download
M runtime/vm/object_test.cc View 2 chunks +70 lines, -3 lines 2 comments Download
M sdk/lib/_internal/compiler/implementation/js_backend/constant_emitter.dart View 1 chunk +4 lines, -15 lines 0 comments Download
M sdk/lib/_internal/compiler/implementation/lib/js_helper.dart View 1 chunk +18 lines, -0 lines 0 comments Download
M sdk/lib/_internal/compiler/implementation/lib/js_string.dart View 1 chunk +1 line, -1 line 0 comments Download
M sdk/lib/_internal/compiler/implementation/source_map_builder.dart View 1 chunk +1 line, -1 line 0 comments Download
M sdk/lib/_internal/compiler/implementation/string_validator.dart View 2 chunks +19 lines, -5 lines 0 comments Download
M sdk/lib/_internal/compiler/implementation/util/characters.dart View 1 chunk +8 lines, -0 lines 0 comments Download
M sdk/lib/_internal/compiler/implementation/util/util.dart View 1 chunk +64 lines, -40 lines 0 comments Download
M sdk/lib/core/string.dart View 1 chunk +7 lines, -4 lines 0 comments Download
M sdk/lib/io/string_stream.dart View 1 chunk +5 lines, -30 lines 0 comments Download
M sdk/lib/utf/utf16.dart View 1 2 4 chunks +7 lines, -35 lines 0 comments Download
M sdk/lib/utf/utf_core.dart View 2 chunks +2 lines, -27 lines 0 comments Download
M tests/co19/co19-dart2js.status View 1 1 chunk +2 lines, -3 lines 0 comments Download
M tests/compiler/dart2js/constant_folding_test.dart View 1 chunk +1 line, -1 line 0 comments Download
M tests/corelib/corelib.status View 4 chunks +10 lines, -1 line 0 comments Download
M tests/corelib/string_from_list_test.dart View 1 chunk +9 lines, -4 lines 0 comments Download
M tests/language/language_dart2js.status View 1 chunk +0 lines, -1 line 0 comments Download
M tests/standalone/io/string_stream_test.dart View 1 chunk +2 lines, -2 lines 0 comments Download
M tests/utils/uri_test.dart View 1 chunk +3 lines, -2 lines 0 comments Download
M tests/utils/utf8_test.dart View 2 chunks +6 lines, -10 lines 0 comments Download
M tests/utils/utils.status View 1 chunk +0 lines, -1 line 0 comments Download
M utils/tests/string_encoding/unicode_test.dart View 1 chunk +10 lines, -10 lines 0 comments Download

Messages

Total messages: 4 (0 generated)
erikcorry
8 years, 1 month ago (2012-11-21 15:31:10 UTC) #1
floitsch
LGTM. https://codereview.chromium.org/11418115/diff/1/sdk/lib/_internal/compiler/implementation/js_backend/constant_emitter.dart File sdk/lib/_internal/compiler/implementation/js_backend/constant_emitter.dart (left): https://codereview.chromium.org/11418115/diff/1/sdk/lib/_internal/compiler/implementation/js_backend/constant_emitter.dart#oldcode94 sdk/lib/_internal/compiler/implementation/js_backend/constant_emitter.dart:94: Node diagnosticNode) { woot! no diagnostic node anymore ...
8 years, 1 month ago (2012-11-21 16:16:50 UTC) #2
cshapiro
https://codereview.chromium.org/11418115/diff/8001/runtime/vm/object_test.cc File runtime/vm/object_test.cc (right): https://codereview.chromium.org/11418115/diff/8001/runtime/vm/object_test.cc#newcode1594 runtime/vm/object_test.cc:1594: EXPECT_EQ(monkey.raw(), Symbols::New("🐵")); Please do not put non-ASCII characters in ...
8 years, 1 month ago (2012-11-21 18:13:23 UTC) #3
erikcorry
8 years, 1 month ago (2012-11-22 12:42:15 UTC) #4 
https://codereview.chromium.org/11418115/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/lib/js_helper.dart (right):

https://codereview.chromium.org/11418115/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:543: JS('void',
r'#.push(#)', a, i);
On 2012/11/21 16:16:50, floitsch wrote:
> a.add(i) should be fine. We know the type of the array so this should be
> compiled perfectly.

It's not fine, but i changed it back to just use add(), and I will file a
separate bug to fix that.

https://codereview.chromium.org/11418115/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/string_validator.dart (right):

https://codereview.chromium.org/11418115/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/string_validator.dart:94: //
terminators in non-multiline strings, and for invalid Unicode
On 2012/11/21 16:16:50, floitsch wrote:
> Update comment.

Done.

https://codereview.chromium.org/11418115/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/util/util.dart (right):

https://codereview.chromium.org/11418115/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/util/util.dart:33: void
writeJsonEscapedCharsOn(String string, buffer) {
On 2012/11/21 16:16:50, floitsch wrote:
> var buffer

Done.

https://codereview.chromium.org/11418115/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/util/util.dart:49: void
writeEscaped(String string, buffer) {
On 2012/11/21 16:16:50, floitsch wrote:
> writeEscapedOn (or change name above)

Done.

https://codereview.chromium.org/11418115/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/util/util.dart:52: if (identical(code,
$DQ)) {
On 2012/11/21 16:16:50, floitsch wrote:
> nyc: remove identicals.
> numbers should never be compared with identical.

Done.

https://codereview.chromium.org/11418115/diff/8001/runtime/vm/object_test.cc
File runtime/vm/object_test.cc (right):

https://codereview.chromium.org/11418115/diff/8001/runtime/vm/object_test.cc#...
runtime/vm/object_test.cc:1594: EXPECT_EQ(monkey.raw(), Symbols::New("🐵"));
On 2012/11/21 18:13:23, cshapiro wrote:
> Please do not put non-ASCII characters in the VM sources.  Escaped UTF-8 is
> okay.

Done.

Powered by Google App Engine
This is Rietveld 408576698