Handle BOMs in dart2js.

Handle BOMs in dart2js.

BUG= http://dartbug.com/15984
R=ahe@google.com

Committed: https://code.google.com/p/dart/source/detail?r=32013

[ahe, 2014-01-14 14:23:59]

LGTM!

https://codereview.chromium.org/137343002/diff/50001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/scanner/utf8_bytes_scanner.dart
(right):

https://codereview.chromium.org/137343002/diff/50001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/scanner/utf8_bytes_scanner.dart:147:
codePoint = "\u{FEFF}";
Can you use UNICODE_BOM_CHARACTER_RUNE somehow?

[floitsch, 2014-01-14 18:37:58]

https://codereview.chromium.org/137343002/diff/50001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/scanner/utf8_bytes_scanner.dart
(right):

https://codereview.chromium.org/137343002/diff/50001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/scanner/utf8_bytes_scanner.dart:147:
codePoint = "\u{FEFF}";
On 2014/01/14 14:23:59, ahe wrote:
> Can you use UNICODE_BOM_CHARACTER_RUNE somehow?

I don't think so.
I don't want to recreate a string every time. I could add an assert, though:
assert(codePoint == new String.fromCharCode(UNICODE_BOM_CHARACTER_RUNE));

should I?

[ahe, 2014-01-15 13:15:37]

https://codereview.chromium.org/137343002/diff/50001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/scanner/utf8_bytes_scanner.dart
(right):

https://codereview.chromium.org/137343002/diff/50001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/scanner/utf8_bytes_scanner.dart:103:
assert(() {
I would probably add a unit test for this instead of an assert.

If you want a fast scanner, it should be so fast that it is done before an
optimizing JIT notices what is going on. This means that you should keep the
code simple, for example, by keeping the number of tokens low.

We know that dart2js performance is affected by how large the snapshot is
(because there will be less heap space available and more GCs).  By being frugal
about asserts, you help keep the snapshot smaller.

https://codereview.chromium.org/137343002/diff/50001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/scanner/utf8_bytes_scanner.dart:147:
codePoint = "\u{FEFF}";
On 2014/01/14 18:37:58, floitsch wrote:
> On 2014/01/14 14:23:59, ahe wrote:
> > Can you use UNICODE_BOM_CHARACTER_RUNE somehow?
> 
> I don't think so.

I would expect that it is extremely rare if a file contains more than zero BOM.

> I don't want to recreate a string every time. I could add an assert, though:
> assert(codePoint == new String.fromCharCode(UNICODE_BOM_CHARACTER_RUNE));
> 
> should I?

I would suggest writing a test instead of using asserts.

[floitsch, 2014-01-15 13:44:46]

If you want TAL.
I will commit this at the end of this afternoon.

https://codereview.chromium.org/137343002/diff/50001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/scanner/utf8_bytes_scanner.dart
(right):

https://codereview.chromium.org/137343002/diff/50001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/scanner/utf8_bytes_scanner.dart:103:
assert(() {
On 2014/01/15 13:15:38, ahe wrote:
> I would probably add a unit test for this instead of an assert.
> 
> If you want a fast scanner, it should be so fast that it is done before an
> optimizing JIT notices what is going on. This means that you should keep the
> code simple, for example, by keeping the number of tokens low.
> 
> We know that dart2js performance is affected by how large the snapshot is
> (because there will be less heap space available and more GCs).  By being
frugal
> about asserts, you help keep the snapshot smaller.

There is already a test.
Removing the assert.

https://codereview.chromium.org/137343002/diff/50001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/scanner/utf8_bytes_scanner.dart:147:
codePoint = "\u{FEFF}";
On 2014/01/15 13:15:38, ahe wrote:
> On 2014/01/14 18:37:58, floitsch wrote:
> > On 2014/01/14 14:23:59, ahe wrote:
> > > Can you use UNICODE_BOM_CHARACTER_RUNE somehow?
> > 
> > I don't think so.
> 
> I would expect that it is extremely rare if a file contains more than zero
BOM.
> 
> > I don't want to recreate a string every time. I could add an assert, though:
> > assert(codePoint == new String.fromCharCode(UNICODE_BOM_CHARACTER_RUNE));
> > 
> > should I?
> 
> I would suggest writing a test instead of using asserts.

I will allocate a new string.
I prefer keeping the assert here. It is a code-path that is executed very
rarely.
Test already exists.

[ahe, 2014-01-15 13:54:23]

Still LGTM :-)

https://codereview.chromium.org/137343002/diff/50001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/scanner/utf8_bytes_scanner.dart
(right):

https://codereview.chromium.org/137343002/diff/50001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/scanner/utf8_bytes_scanner.dart:147:
codePoint = "\u{FEFF}";
On 2014/01/15 13:44:46, floitsch wrote:
> On 2014/01/15 13:15:38, ahe wrote:
> > On 2014/01/14 18:37:58, floitsch wrote:
> > > On 2014/01/14 14:23:59, ahe wrote:
> > > > Can you use UNICODE_BOM_CHARACTER_RUNE somehow?
> > > 
> > > I don't think so.
> > 
> > I would expect that it is extremely rare if a file contains more than zero
> BOM.
> > 
> > > I don't want to recreate a string every time. I could add an assert,
though:
> > > assert(codePoint == new String.fromCharCode(UNICODE_BOM_CHARACTER_RUNE));
> > > 
> > > should I?
> > 
> > I would suggest writing a test instead of using asserts.
> 
> I will allocate a new string.
> I prefer keeping the assert here. It is a code-path that is executed very
> rarely.

Yes. Keep the assert on line 146. That assert tests an invariant about code that
is out of your control.

Alternatively, turn it into an internal error.

> Test already exists.

My suggestion is this:

const String UNICODE_BOM_CHARACTER = "\u{FEFF}";

And then this test:

main() {
  Expect.stringEquals(new String.fromCharCode(UNICODE_BOM_CHARACTER_RUNE),
UNICODE_BOM_CHARACTER);
}

[floitsch, 2014-01-15 14:51:38]

https://codereview.chromium.org/137343002/diff/50001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/scanner/utf8_bytes_scanner.dart
(right):

https://codereview.chromium.org/137343002/diff/50001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/scanner/utf8_bytes_scanner.dart:147:
codePoint = "\u{FEFF}";
On 2014/01/15 13:54:23, ahe wrote:
> On 2014/01/15 13:44:46, floitsch wrote:
> > On 2014/01/15 13:15:38, ahe wrote:
> > > On 2014/01/14 18:37:58, floitsch wrote:
> > > > On 2014/01/14 14:23:59, ahe wrote:
> > > > > Can you use UNICODE_BOM_CHARACTER_RUNE somehow?
> > > > 
> > > > I don't think so.
> > > 
> > > I would expect that it is extremely rare if a file contains more than zero
> > BOM.
> > > 
> > > > I don't want to recreate a string every time. I could add an assert,
> though:
> > > > assert(codePoint == new
String.fromCharCode(UNICODE_BOM_CHARACTER_RUNE));
> > > > 
> > > > should I?
> > > 
> > > I would suggest writing a test instead of using asserts.
> > 
> > I will allocate a new string.
> > I prefer keeping the assert here. It is a code-path that is executed very
> > rarely.
> 
> Yes. Keep the assert on line 146. That assert tests an invariant about code
that
> is out of your control.
> 
> Alternatively, turn it into an internal error.
> 
> > Test already exists.
> 
> My suggestion is this:
> 
> const String UNICODE_BOM_CHARACTER = "\u{FEFF}";
> 
> And then this test:
> 
> main() {
>   Expect.stringEquals(new String.fromCharCode(UNICODE_BOM_CHARACTER_RUNE),
> UNICODE_BOM_CHARACTER);
> }

I see.
As you say: there shouldn't be many BOMs inside the code. So keeping the simpler
new String.fromCharCode.

[floitsch, 2014-01-27 10:42:58]

Committed patchset #4 manually as r32013 (presubmit successful).