Handle escapes in string literals.

Handle escapes in string literals.

Also handles raw strings.

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

[Lasse Reichstein, 2012-01-12 11:20:39]

Peter, please check the scanner changes.

[ahe, 2012-01-12 12:06:29]

This makes the scanner slightly slower. Let's talk about alternatives
face-to-face.

[floitsch, 2012-01-12 12:30:28]

LGTM with many nits.
If you disagree talk to me, to make sure it's not one of the things I feel
strongly about.

http://codereview.chromium.org/9190038/diff/1/frog/leg/ssa/nodes.dart
File frog/leg/ssa/nodes.dart (right):

http://codereview.chromium.org/9190038/diff/1/frog/leg/ssa/nodes.dart#newcode...
frog/leg/ssa/nodes.dart:1559: return hexDigit - 0x30;
Don't use random constants. I would prefer not to do these optimizations before
we actually need them, but at the very least assert that you are right:
int nineCharCode = 0x39;
assert("9".charCodeAt(0) == nineCharCode);

http://codereview.chromium.org/9190038/diff/1/frog/leg/ssa/nodes.dart#newcode...
frog/leg/ssa/nodes.dart:1568: /* Write the contents of the quoted string to a
[StringBuffer] in
/**

http://codereview.chromium.org/9190038/diff/1/frog/leg/ssa/nodes.dart#newcode...
frog/leg/ssa/nodes.dart:1588: buffer.add(@'\');
Don't you need to update from?

http://codereview.chromium.org/9190038/diff/1/frog/leg/ssa/nodes.dart#newcode...
frog/leg/ssa/nodes.dart:1590: character = wrappedString[++i];
I find it easier to i++; first:
i++;
character = wrappedString[i];

http://codereview.chromium.org/9190038/diff/1/frog/leg/ssa/nodes.dart#newcode...
frog/leg/ssa/nodes.dart:1592: if (wrappedString[++i] == '{') {
ditto

http://codereview.chromium.org/9190038/diff/1/frog/leg/ssa/nodes.dart#newcode...
frog/leg/ssa/nodes.dart:1592: if (wrappedString[++i] == '{') {
Maybe move the \u{...} section into a (local/nested?) method.

http://codereview.chromium.org/9190038/diff/1/frog/leg/ssa/nodes.dart#newcode...
frog/leg/ssa/nodes.dart:1597: int hexDigit = wrappedString.charCodeAt(++i);
ditto. (maybe also move i++ up, so that it is clear that '{' has been consumed)
use i++;

http://codereview.chromium.org/9190038/diff/1/frog/leg/ssa/nodes.dart#newcode...
frog/leg/ssa/nodes.dart:1600: hexDigit = wrappedString.charCodeAt(++i);
use i++

http://codereview.chromium.org/9190038/diff/1/frog/leg/ssa/nodes.dart#newcode...
frog/leg/ssa/nodes.dart:1601: } while (hexDigit !== 0x7d);  // until '}'.
magic value.

http://codereview.chromium.org/9190038/diff/1/frog/leg/ssa/nodes.dart#newcode...
frog/leg/ssa/nodes.dart:1602: // JavaScript only supports characters in the BMP.
That's not true. It allows for the full UTF-16. We should probably generate a
surrogate pair.
At the very least add a TODO, but I would either implement it, or bailout.

http://codereview.chromium.org/9190038/diff/1/frog/leg/ssa/nodes.dart#newcode...
frog/leg/ssa/nodes.dart:1625: // with the same meaning, so we include them
verbatim.
I would still prefer to have them explicitly handled: one section dealing with
identical special escapes, and one where we can remove the '\' (identity
escapes). Otherwise we have to trust your comment...

http://codereview.chromium.org/9190038/diff/1/frog/tests/leg/src/BuiltinInter...
File frog/tests/leg/src/BuiltinInterceptorTest.dart (right):

http://codereview.chromium.org/9190038/diff/1/frog/tests/leg/src/BuiltinInter...
frog/tests/leg/src/BuiltinInterceptorTest.dart:37: } catch (Exception e) {
why this change?

http://codereview.chromium.org/9190038/diff/1/frog/tests/leg_only/src/StringE...
File frog/tests/leg_only/src/StringEscapeTest.dart (right):

http://codereview.chromium.org/9190038/diff/1/frog/tests/leg_only/src/StringE...
frog/tests/leg_only/src/StringEscapeTest.dart:80: var nonMBR = "\u{10000}";
I'm not sure this is the correct approach.

[Lasse Reichstein Nielsen, 2012-01-16 13:30:23]

Rewrite to use SourceString iterators.

http://codereview.chromium.org/9190038/diff/1/frog/leg/ssa/nodes.dart
File frog/leg/ssa/nodes.dart (right):

http://codereview.chromium.org/9190038/diff/1/frog/leg/ssa/nodes.dart#newcode...
frog/leg/ssa/nodes.dart:1559: return hexDigit - 0x30;
Now uses $9, etc.

http://codereview.chromium.org/9190038/diff/1/frog/leg/ssa/nodes.dart#newcode...
frog/leg/ssa/nodes.dart:1568: /* Write the contents of the quoted string to a
[StringBuffer] in
On 2012/01/12 12:30:28, floitsch wrote:
> /**

Done.

http://codereview.chromium.org/9190038/diff/1/frog/leg/ssa/nodes.dart#newcode...
frog/leg/ssa/nodes.dart:1588: buffer.add(@'\');
I did. It's now completely rewritten.

http://codereview.chromium.org/9190038/diff/1/frog/leg/ssa/nodes.dart#newcode...
frog/leg/ssa/nodes.dart:1590: character = wrappedString[++i];
In many cases I'd agree. In this, I actually read the entire thing as a simple
iteration through the wrappedString.
Anyway, it's gone now.

http://codereview.chromium.org/9190038/diff/1/frog/leg/ssa/nodes.dart#newcode...
frog/leg/ssa/nodes.dart:1592: if (wrappedString[++i] == '{') {
Moved.

http://codereview.chromium.org/9190038/diff/1/frog/leg/ssa/nodes.dart#newcode...
frog/leg/ssa/nodes.dart:1597: int hexDigit = wrappedString.charCodeAt(++i);
gone.

http://codereview.chromium.org/9190038/diff/1/frog/leg/ssa/nodes.dart#newcode...
frog/leg/ssa/nodes.dart:1625: // with the same meaning, so we include them
verbatim.
Done.

http://codereview.chromium.org/9190038/diff/1/frog/tests/leg/src/BuiltinInter...
File frog/tests/leg/src/BuiltinInterceptorTest.dart (right):

http://codereview.chromium.org/9190038/diff/1/frog/tests/leg/src/BuiltinInter...
frog/tests/leg/src/BuiltinInterceptorTest.dart:37: } catch (Exception e) {
No good reason. I removed the wrapper because I thought it worked, and when it
didn't I wrote it again from scratch.

Either way works.

http://codereview.chromium.org/9190038/diff/1/frog/tests/leg_only/src/StringE...
File frog/tests/leg_only/src/StringEscapeTest.dart (right):

http://codereview.chromium.org/9190038/diff/1/frog/tests/leg_only/src/StringE...
frog/tests/leg_only/src/StringEscapeTest.dart:80: var nonMBR = "\u{10000}";
Not in the long run. 
A more correct approach is far more complicated, so this will have to do for now
(we can't just expand to a surrogate pair because we still use the JS String
.length for string length, so we need something that's not a JS string to hold
it).

[floitsch, 2012-01-16 14:29:50]

LGTM.
consider moving the characters lib into another directory.

http://codereview.chromium.org/9190038/diff/6001/frog/leg/ssa/nodes.dart
File frog/leg/ssa/nodes.dart (right):

http://codereview.chromium.org/9190038/diff/6001/frog/leg/ssa/nodes.dart#newc...
frog/leg/ssa/nodes.dart:1660: hexDigit |= $a - $A;
This "trick" depends on an ascii property where lower/upper case is just
expressed by a bit. The x |= $a - $A would not (necessarily) work in other
encodings. Maybe:

assert((hexDigit | 0x20) == hexDigit < $a ? hexDigit + $a - $A : hexDigit);
hexDigit |= 0x20;

There is a magic constant, but at least it shows what code would have been there
to without the trick.

http://codereview.chromium.org/9190038/diff/6001/frog/leg/ssa/nodes.dart#newc...
frog/leg/ssa/nodes.dart:1678: } while (hexDigit !== $CLOSE_CURLY_BRACKET);  //
until '}'.
remove comment.

http://codereview.chromium.org/9190038/diff/6001/frog/leg/ssa/nodes.dart#newc...
frog/leg/ssa/nodes.dart:1682: if (value > 0xffff) value = 0xfffd;
I would prefer bailing out (compiler.cancel).

[Lasse Reichstein Nielsen, 2012-01-17 08:50:51]

Moved characters.dart to leg/util

http://codereview.chromium.org/9190038/diff/6001/frog/leg/ssa/nodes.dart
File frog/leg/ssa/nodes.dart (right):

http://codereview.chromium.org/9190038/diff/6001/frog/leg/ssa/nodes.dart#newc...
frog/leg/ssa/nodes.dart:1660: hexDigit |= $a - $A;
I assume Unicode. There are no other encodings. FINI! 

Everything else in this method assume Unicode too (as does the characters.dart
file), so checking just this little bit seems spurious.

I've changed it to 
  |= $a ^ $A;
instead. More readable, arguably.

http://codereview.chromium.org/9190038/diff/6001/frog/leg/ssa/nodes.dart#newc...
frog/leg/ssa/nodes.dart:1678: } while (hexDigit !== $CLOSE_CURLY_BRACKET);  //
until '}'.
Done.

http://codereview.chromium.org/9190038/diff/6001/frog/leg/ssa/nodes.dart#newc...
frog/leg/ssa/nodes.dart:1682: if (value > 0xffff) value = 0xfffd;
Done.

[ahe, 2012-01-17 13:21:38]

Scanner and test changes LGTM, but as we discussed offline, the scanner bench
suffers from this change, so it is better to validate strings in the parser.

http://codereview.chromium.org/9190038/diff/10001/frog/leg/scanner/scanner.dart
File frog/leg/scanner/scanner.dart (right):

http://codereview.chromium.org/9190038/diff/10001/frog/leg/scanner/scanner.da...
frog/leg/scanner/scanner.dart:695: if (next === $EOF || next === $LF || next ===
$CR) {
Shouldn't this be in tokenizeStringEscape?

http://codereview.chromium.org/9190038/diff/13001/frog/tests/leg/src/BuiltinI...
File frog/tests/leg/src/BuiltinInterceptorTest.dart (right):

http://codereview.chromium.org/9190038/diff/13001/frog/tests/leg/src/BuiltinI...
frog/tests/leg/src/BuiltinInterceptorTest.dart:37: } catch (Exception e) {
This seems unfortunate to me. How about:

generated = compile(TEST_TWO, 'foo');
Expect.isFalse(generated.contains("return 3;"));// TODO(lrn): Switch to isTrue.

...

http://codereview.chromium.org/9190038/diff/13001/frog/tests/leg_only/src/Str...
File frog/tests/leg_only/src/StringEscapeTest.dart (right):

http://codereview.chromium.org/9190038/diff/13001/frog/tests/leg_only/src/Str...
frog/tests/leg_only/src/StringEscapeTest.dart:1: // Copyright (c) 2011, the Dart
project authors.  Please see the AUTHORS file
This comment is so last year.

http://codereview.chromium.org/9190038/diff/13001/frog/tests/leg_only/src/Str...
frog/tests/leg_only/src/StringEscapeTest.dart:2: 
Extra line.

http://codereview.chromium.org/9190038/diff/13001/frog/tests/leg_only/src/Str...
frog/tests/leg_only/src/StringEscapeTest.dart:6: // Test that string escapes
work correctly.
Move this test to language?

[Lasse Reichstein Nielsen, 2012-01-18 08:22:34]

http://codereview.chromium.org/9190038/diff/10001/frog/leg/scanner/scanner.dart
File frog/leg/scanner/scanner.dart (right):

http://codereview.chromium.org/9190038/diff/10001/frog/leg/scanner/scanner.da...
frog/leg/scanner/scanner.dart:695: if (next === $EOF || next === $LF || next ===
$CR) {
This method has been reverted.

This shouldn't have been in tokenizeStringEscape, since I wanted to reuse it for
multiline strings (where $LF and $CR are valid).

http://codereview.chromium.org/9190038/diff/13001/frog/tests/leg/src/BuiltinI...
File frog/tests/leg/src/BuiltinInterceptorTest.dart (right):

http://codereview.chromium.org/9190038/diff/13001/frog/tests/leg/src/BuiltinI...
frog/tests/leg/src/BuiltinInterceptorTest.dart:37: } catch (Exception e) {
Good idea.

http://codereview.chromium.org/9190038/diff/13001/frog/tests/leg_only/src/Str...
File frog/tests/leg_only/src/StringEscapeTest.dart (right):

http://codereview.chromium.org/9190038/diff/13001/frog/tests/leg_only/src/Str...
frog/tests/leg_only/src/StringEscapeTest.dart:1: // Copyright (c) 2011, the Dart
project authors.  Please see the AUTHORS file
Fixed.

http://codereview.chromium.org/9190038/diff/13001/frog/tests/leg_only/src/Str...
frog/tests/leg_only/src/StringEscapeTest.dart:2: 
Cut.

http://codereview.chromium.org/9190038/diff/13001/frog/tests/leg_only/src/Str...
frog/tests/leg_only/src/StringEscapeTest.dart:6: // Test that string escapes
work correctly.
Let's keep it here until we can tests for characters > 0xffff too (i.e., when
Leg supports it).
Also, it seems the VM only accepts six hex digits in a \u{...} escape. 
Seems like it's probably a bug in the spec to allow seven.