Update handling of multiline strings to specification.

Update handling of multiline strings to specification.

R=johnniwinther@google.com, lrn@google.com

Committed: https://github.com/dart-lang/sdk/commit/0dba8c951691159e36f8367be395b99c672497b6

[ahe, 2017-04-27 15:33:57]

ahe@google.com changed reviewers:
+ johnniwinther@google.com, lrn@google.com

[Lasse Reichstein Nielsen, 2017-04-28 06:34:28]

lgtm

https://codereview.chromium.org/2847793002/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/quote.dart (right):

https://codereview.chromium.org/2847793002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/quote.dart:61: if (code == $BACKSLASH) {
I REALLY think we should disallow quoting leading whitespace. It makes *no*
sense.
Why is `\ ` ignored but `\x20` not? 
WHY?!?

ahem.

https://codereview.chromium.org/2847793002/diff/1/tests/language/multiline_ne...
File tests/language/multiline_newline_test.dart (right):

https://codereview.chromium.org/2847793002/diff/1/tests/language/multiline_ne...
tests/language/multiline_newline_test.dart:58: : null;
Why the `? true : null` followed by `Expect.isTrue`. 
You can change null to false without changing the result, and then it's just the
same as not having the conditional operator at all.

So, suggestion: Remove the `? true : null`. Ditto everywhere, just change
.isNull to isFalse.

https://codereview.chromium.org/2847793002/diff/1/tests/language/multiline_ne...
tests/language/multiline_newline_test.dart:79: Expect.equals(1, c4r); //  //#
01r: continued
Comment indentation off by one :) 
More below.

https://codereview.chromium.org/2847793002/diff/1/tests/language/multiline_ne...
tests/language/multiline_newline_test.dart:110: const c10 = c7 ? 1 : 2; // //#
04: compile-time error
What is this testing? That `null` cannot be used as a conditional operator
condition in a const expression? That seems cute, but not really on-topic for
this test.

[Johnni Winther, 2017-04-28 07:12:01]

lgtm

https://codereview.chromium.org/2847793002/diff/1/tests/language/multiline_ne...
File tests/language/multiline_newline_test.dart (right):

https://codereview.chromium.org/2847793002/diff/1/tests/language/multiline_ne...
tests/language/multiline_newline_test.dart:58: : null;
On 2017/04/28 06:34:28, Lasse Reichstein Nielsen wrote:
> Why the `? true : null` followed by `Expect.isTrue`. 
> You can change null to false without changing the result, and then it's just
the
> same as not having the conditional operator at all.
> 
> So, suggestion: Remove the `? true : null`. Ditto everywhere, just change
> .isNull to isFalse.

For analyzer testing. See lines 110-116

https://codereview.chromium.org/2847793002/diff/1/tests/language/multiline_ne...
tests/language/multiline_newline_test.dart:110: const c10 = c7 ? 1 : 2; // //#
04: compile-time error
On 2017/04/28 06:34:28, Lasse Reichstein Nielsen wrote:
> What is this testing? That `null` cannot be used as a conditional operator
> condition in a const expression? That seems cute, but not really on-topic for
> this test. 

This is to validate the analyzer implementation. It doesn't 'run' the code
unless it is in constants.

[ahe, 2017-04-28 07:39:00]

Thank you, Lasse and Johnni.

I think the conclusion is that this CL should be landed as is modulo some
comment indentation.

https://codereview.chromium.org/2847793002/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/quote.dart (right):

https://codereview.chromium.org/2847793002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/quote.dart:61: if (code == $BACKSLASH) {
On 2017/04/28 06:34:28, Lasse Reichstein Nielsen wrote:
> I REALLY think we should disallow quoting leading whitespace. It makes *no*
> sense.
> Why is `\ ` ignored but `\x20` not? 
> WHY?!?
> 
> ahem.

Acknowledged.

https://codereview.chromium.org/2847793002/diff/1/tests/language/multiline_ne...
File tests/language/multiline_newline_test.dart (right):

https://codereview.chromium.org/2847793002/diff/1/tests/language/multiline_ne...
tests/language/multiline_newline_test.dart:79: Expect.equals(1, c4r); //  //#
01r: continued
On 2017/04/28 06:34:28, Lasse Reichstein Nielsen wrote:
> Comment indentation off by one :) 
> More below.

Acknowledged.

https://codereview.chromium.org/2847793002/diff/1/tests/language/multiline_ne...
tests/language/multiline_newline_test.dart:110: const c10 = c7 ? 1 : 2; // //#
04: compile-time error
On 2017/04/28 07:12:01, Johnni Winther wrote:
> On 2017/04/28 06:34:28, Lasse Reichstein Nielsen wrote:
> > What is this testing? That `null` cannot be used as a conditional operator
> > condition in a const expression? That seems cute, but not really on-topic
for
> > this test. 
> 
> This is to validate the analyzer implementation. It doesn't 'run' the code
> unless it is in constants.

Not only does this validate the analyzer implementation, it also tests dart2js'
compile-time constant evaluation which is implemented differently from its
runtime evaluation.

[Lasse Reichstein Nielsen, 2017-04-28 08:57:36]

https://codereview.chromium.org/2847793002/diff/1/tests/language/multiline_ne...
File tests/language/multiline_newline_test.dart (right):

https://codereview.chromium.org/2847793002/diff/1/tests/language/multiline_ne...
tests/language/multiline_newline_test.dart:110: const c10 = c7 ? 1 : 2; // //#
04: compile-time error
So the null is there to force a compile-time error below, which a bool wouldn't
do, and it's the only way to get a visible result out of the analyzer?

It's still feels a little (too) clever, but I can see the reasoning.
Just switching the values around to only get a compile-time error for an
incorrect implementation isn't as visible, it could hide that the entire program
was garbage collected instead.

[Johnni Winther, 2017-04-28 09:19:44]

https://codereview.chromium.org/2847793002/diff/1/tests/language/multiline_ne...
File tests/language/multiline_newline_test.dart (right):

https://codereview.chromium.org/2847793002/diff/1/tests/language/multiline_ne...
tests/language/multiline_newline_test.dart:110: const c10 = c7 ? 1 : 2; // //#
04: compile-time error
On 2017/04/28 08:57:36, Lasse Reichstein Nielsen wrote:
> So the null is there to force a compile-time error below, which a bool
wouldn't
> do, and it's the only way to get a visible result out of the analyzer?
> 
> It's still feels a little (too) clever, but I can see the reasoning.
> Just switching the values around to only get a compile-time error for an
> incorrect implementation isn't as visible, it could hide that the entire
program
> was garbage collected instead.

We should probably add a comment about the intent of these compile-time errors.

[ahe, 2017-05-08 08:36:18]

Thank you, Johnni and Lasse!

https://codereview.chromium.org/2847793002/diff/1/tests/language/multiline_ne...
File tests/language/multiline_newline_test.dart (right):

https://codereview.chromium.org/2847793002/diff/1/tests/language/multiline_ne...
tests/language/multiline_newline_test.dart:79: Expect.equals(1, c4r); //  //#
01r: continued
On 2017/04/28 07:39:00, ahe wrote:
> On 2017/04/28 06:34:28, Lasse Reichstein Nielsen wrote:
> > Comment indentation off by one :) 
> > More below.
> 
> Acknowledged.

Done.

https://codereview.chromium.org/2847793002/diff/1/tests/language/multiline_ne...
tests/language/multiline_newline_test.dart:110: const c10 = c7 ? 1 : 2; // //#
04: compile-time error
On 2017/04/28 09:19:43, Johnni Winther wrote:
> On 2017/04/28 08:57:36, Lasse Reichstein Nielsen wrote:
> > So the null is there to force a compile-time error below, which a bool
> wouldn't
> > do, and it's the only way to get a visible result out of the analyzer?
> > 
> > It's still feels a little (too) clever, but I can see the reasoning.
> > Just switching the values around to only get a compile-time error for an
> > incorrect implementation isn't as visible, it could hide that the entire
> program
> > was garbage collected instead.
> 
> We should probably add a comment about the intent of these compile-time
errors.

Done.

[ahe, 2017-05-08 10:20:11]

Description was changed from

==========
Update handling of multiline strings to specification.
==========

to

==========
Update handling of multiline strings to specification.

R=johnniwinther@google.com, lrn@google.com

Committed:
https://github.com/dart-lang/sdk/commit/0dba8c951691159e36f8367be395b99c672497b6
==========

[ahe, 2017-05-08 10:20:12]

Committed patchset #3 (id:40001) manually as
0dba8c951691159e36f8367be395b99c672497b6 (presubmit successful).