Allow whitespace and \ before the first newline of multiline string.

Allow whitespace and \ before the first newline of multiline string.

According to the spec leading whitespace and \ is discarded if it is followed by a newline.

BUG= http://dartbug.com/19386
R=johnniwinther@google.com, lrn@google.com

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

[Lasse Reichstein Nielsen, 2014-06-17 15:56:52]

https://codereview.chromium.org/336413002/diff/80001/tests/compiler/dart2js/s...
File tests/compiler/dart2js/string_escapes_test.dart (right):

https://codereview.chromium.org/336413002/diff/80001/tests/compiler/dart2js/s...
tests/compiler/dart2js/string_escapes_test.dart:32: }),
Also test backslashes before other whitespace characters like
  "r'''\\\t\n'''"

[ahe, 2014-06-18 07:54:25]

If possible, get an l*g*t*m from Lasse. IIRC, he wrote this code originally.

I skimmed the code and found I couldn't add much.

[Johnni Winther, 2014-06-18 08:43:44]

https://codereview.chromium.org/336413002/diff/80001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/string_validator.dart (right):

https://codereview.chromium.org/336413002/diff/80001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/string_validator.dart:39: int
quoteLength = 1;
Rename to [leftQuoteLength]. Also in StringQuoting.getQuoting.

https://codereview.chromium.org/336413002/diff/80001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/string_validator.dart:89: return
StringQuoting.getQuoting(quoteChar, raw, quoteLength);
StringQuoting.getQuoting expects [quoteLength] to be limited.

https://codereview.chromium.org/336413002/diff/80001/tests/compiler/dart2js/s...
File tests/compiler/dart2js/string_escapes_test.dart (right):

https://codereview.chromium.org/336413002/diff/80001/tests/compiler/dart2js/s...
tests/compiler/dart2js/string_escapes_test.dart:23:
Expect.isTrue(generated.contains(r"\r\u2028\u2029"));
Add tests for \r, \n, \r\n adn \n\r.

[Lasse Reichstein Nielsen, 2014-06-18 09:05:32]

LGTM with more tests.

https://codereview.chromium.org/336413002/diff/80001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/string_validator.dart (right):

https://codereview.chromium.org/336413002/diff/80001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/string_validator.dart:68:
foundWhitespace = false;
This "break variable" is more confusing than a "while(true)" with explicit
breaks on newlines or non-whitespace.

If you want to keep it, just set it to true to begin with, and rename it to
"foundOnlyWhitespace" (or reverese its meaning as "foundNonWhitespace"), and
only set it to false in a final else branch, and use "break" in the
line-terminator cases. No need to change it back and forth each iteration.
Technically the newline cases are "wrong" now, because newlines are whitespace,
but foundWhitespace ends up false after seeing a newline.

https://codereview.chromium.org/336413002/diff/80001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/string_validator.dart:69:
source.moveNext();
Why is the return value of moveNext not checked?
If it works anyway, because a current returning null gives the correct result
anyway, please document it.

https://codereview.chromium.org/336413002/diff/80001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/string_validator.dart:79: } else if
(code == $CR) {
The spec says "If the first line of a multiline ...". What defines a line?
We are using either CRLF, CR or LF as line terminator. 

Ok, not a new issue, I just found http://dartbug.com/14073
We should keep doing what we are doing here: \r | \r\n | \n are line
terminators.

https://codereview.chromium.org/336413002/diff/80001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/string_validator.dart:80: if
(source.moveNext() && source.current == $LF) {
Does the spec require \\\r\\\n to work as a newline sequence? (I'd say "heck
no!", but then I wouldn't have allowed backslashes at all).
I think we can argue that \r\n is the line terminator, and \r\\\n is not, so \r
becomes the line terminator in that case, and the \\\n becomes part of the
string.

Ok, I filed http://dartbug.com/19514 for not allowing backslashes again.

https://codereview.chromium.org/336413002/diff/80001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/tree/nodes.dart (right):

https://codereview.chromium.org/336413002/diff/80001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/tree/nodes.dart:864: const
StringQuoting(this.quote, {bool raw, int leftQuoteLength})
"While you are here": Please change this to "{bool this.raw," and drop the
assignment on the next line.

https://codereview.chromium.org/336413002/diff/80001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/tree/nodes.dart:873: int index =
quoteLength - 1;
Just multiply this by four immediately, instead of doing it twice on the two
following lines.

Also move the computation of quoteKindOFfset and rawOffset into the if body.

https://codereview.chromium.org/336413002/diff/80001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/tree/nodes.dart:877: return new
StringQuoting(quote, raw: raw, leftQuoteLength: index + 1);
index + 1 -> quoteLength

https://codereview.chromium.org/336413002/diff/80001/tests/compiler/dart2js/s...
File tests/compiler/dart2js/string_escapes_test.dart (right):

https://codereview.chromium.org/336413002/diff/80001/tests/compiler/dart2js/s...
tests/compiler/dart2js/string_escapes_test.dart:23:
Expect.isTrue(generated.contains(r"\r\u2028\u2029"));
.. where \n\r is not a line-terminator sequence, so the \r is part of the
string.

The "contains" test doesn't seem to validate that the whitespace-line is omitted
from the string, only that the part after it isn't omitted.

Is there another test that ensures the whitespaces are omitted?

[floitsch, 2014-06-18 12:14:23]

And I thought this was going to be a trivial CL ;)

PTAL.

https://codereview.chromium.org/336413002/diff/80001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/string_validator.dart (right):

https://codereview.chromium.org/336413002/diff/80001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/string_validator.dart:39: int
quoteLength = 1;
On 2014/06/18 08:43:44, Johnni Winther wrote:
> Rename to [leftQuoteLength]. Also in StringQuoting.getQuoting.

Done.

https://codereview.chromium.org/336413002/diff/80001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/string_validator.dart:68:
foundWhitespace = false;
On 2014/06/18 09:05:32, Lasse Reichstein Nielsen wrote:
> This "break variable" is more confusing than a "while(true)" with explicit
> breaks on newlines or non-whitespace.
> 
> If you want to keep it, just set it to true to begin with, and rename it to
> "foundOnlyWhitespace" (or reverese its meaning as "foundNonWhitespace"), and
> only set it to false in a final else branch, and use "break" in the
> line-terminator cases. No need to change it back and forth each iteration.
> Technically the newline cases are "wrong" now, because newlines are
whitespace,
> but foundWhitespace ends up false after seeing a newline.

changed to while(true).
Initially switched away from it, since it ends with a 'break' now.

https://codereview.chromium.org/336413002/diff/80001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/string_validator.dart:69:
source.moveNext();
On 2014/06/18 09:05:32, Lasse Reichstein Nielsen wrote:
> Why is the return value of moveNext not checked?
> If it works anyway, because a current returning null gives the correct result
> anyway, please document it.

Moved comment from line 56 to here.

https://codereview.chromium.org/336413002/diff/80001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/string_validator.dart:89: return
StringQuoting.getQuoting(quoteChar, raw, quoteLength);
On 2014/06/18 08:43:44, Johnni Winther wrote:
> StringQuoting.getQuoting expects [quoteLength] to be limited.

It shouldn't anymore. I changed it to be accept any quoteLength.

https://codereview.chromium.org/336413002/diff/80001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/tree/nodes.dart (right):

https://codereview.chromium.org/336413002/diff/80001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/tree/nodes.dart:864: const
StringQuoting(this.quote, {bool raw, int leftQuoteLength})
On 2014/06/18 09:05:32, Lasse Reichstein Nielsen wrote:
> "While you are here": Please change this to "{bool this.raw," and drop the
> assignment on the next line.

Done.

https://codereview.chromium.org/336413002/diff/80001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/tree/nodes.dart:873: int index =
quoteLength - 1;
On 2014/06/18 09:05:32, Lasse Reichstein Nielsen wrote:
> Just multiply this by four immediately, instead of doing it twice on the two
> following lines.
> 
> Also move the computation of quoteKindOFfset and rawOffset into the if body.

Computing the full index now (including the rawOffset and quoteKindOffset) and
checking against the length.
this way the index < _mapping.length check is saner.

https://codereview.chromium.org/336413002/diff/80001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/tree/nodes.dart:877: return new
StringQuoting(quote, raw: raw, leftQuoteLength: index + 1);
On 2014/06/18 09:05:32, Lasse Reichstein Nielsen wrote:
> index + 1 -> quoteLength

doh.
done.

https://codereview.chromium.org/336413002/diff/80001/tests/compiler/dart2js/s...
File tests/compiler/dart2js/string_escapes_test.dart (right):

https://codereview.chromium.org/336413002/diff/80001/tests/compiler/dart2js/s...
tests/compiler/dart2js/string_escapes_test.dart:23:
Expect.isTrue(generated.contains(r"\r\u2028\u2029"));
On 2014/06/18 08:43:44, Johnni Winther wrote:
> Add tests for \r, \n, \r\n adn \n\r.

Done.

https://codereview.chromium.org/336413002/diff/80001/tests/compiler/dart2js/s...
tests/compiler/dart2js/string_escapes_test.dart:23:
Expect.isTrue(generated.contains(r"\r\u2028\u2029"));
On 2014/06/18 09:05:32, Lasse Reichstein Nielsen wrote:
> .. where \n\r is not a line-terminator sequence, so the \r is part of the
> string.
> 
> The "contains" test doesn't seem to validate that the whitespace-line is
omitted
> from the string, only that the part after it isn't omitted.
> 
> Is there another test that ensures the whitespaces are omitted? 

Done.

https://codereview.chromium.org/336413002/diff/80001/tests/compiler/dart2js/s...
tests/compiler/dart2js/string_escapes_test.dart:32: }),
On 2014/06/17 15:56:52, Lasse Reichstein Nielsen wrote:
> Also test backslashes before other whitespace characters like
>   "r'''\\\t\n'''"

Not tested (and not being accepted):
\\\r\\\n
That is the newline sequence \r\n prefixed by a \.

Done.

[Johnni Winther, 2014-06-18 12:40:01]

lgtm

[floitsch, 2014-06-18 12:59:25]

Forgot to save file.
Waiting for lgtm from Lasse.

[Lasse Reichstein Nielsen, 2014-06-18 14:26:15]

lgtm

[floitsch, 2014-06-18 14:43:44]

Committed patchset #7 manually as r37446 (presubmit successful).