Make string juxtaposition combine properly with string interpolations.

Make string juxtaposition combine properly with string interpolations.

Juxtaposed string literals combine with the first and last string parts
of a string interpolation, to create a new string interpolation.

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

[ngeoffray, 2012-03-08 10:37:08]

LGTM, but I would wait for Peter feedback before submitting.

https://chromiumcodereview.appspot.com/9642001/diff/1/frog/leg/scanner/listen...
File frog/leg/scanner/listener.dart (right):

https://chromiumcodereview.appspot.com/9642001/diff/1/frog/leg/scanner/listen...
frog/leg/scanner/listener.dart:872: if (expression is StringInterpolation) {
expression is -> expression.asStringInterpolation()

https://chromiumcodereview.appspot.com/9642001/diff/1/frog/leg/scanner/listen...
frog/leg/scanner/listener.dart:885: // Accumulator cannot be empty. Both a
literal and in interpolation
in interpolation -> a string part in the interpolated string ?

https://chromiumcodereview.appspot.com/9642001/diff/1/frog/leg/tree/nodes.dart
File frog/leg/tree/nodes.dart (right):

https://chromiumcodereview.appspot.com/9642001/diff/1/frog/leg/tree/nodes.dar...
frog/leg/tree/nodes.dart:729: factory DartString.cons(DartString first,
DartString second) {
cons is very close to 'const' and I often get confused. Could it be something
like concat?

https://chromiumcodereview.appspot.com/9642001/diff/1/frog/leg/tree/nodes.dar...
frog/leg/tree/nodes.dart:734: DartString();
Why do you still have this constructor? There should be a synthethized one
created for you.

[Lasse Reichstein Nielsen, 2012-03-08 11:17:49]

https://chromiumcodereview.appspot.com/9642001/diff/1/frog/leg/scanner/listen...
File frog/leg/scanner/listener.dart (right):

https://chromiumcodereview.appspot.com/9642001/diff/1/frog/leg/scanner/listen...
frog/leg/scanner/listener.dart:872: if (expression is StringInterpolation) {
As discussed offline, I'll keep this one.

https://chromiumcodereview.appspot.com/9642001/diff/1/frog/leg/scanner/listen...
frog/leg/scanner/listener.dart:885: // Accumulator cannot be empty. Both a
literal and in interpolation
Here I'm talking about the expression, which is either a LiteralString or a
StringInterpolation. I'll make it clearer.

https://chromiumcodereview.appspot.com/9642001/diff/1/frog/leg/tree/nodes.dart
File frog/leg/tree/nodes.dart (right):

https://chromiumcodereview.appspot.com/9642001/diff/1/frog/leg/tree/nodes.dar...
frog/leg/tree/nodes.dart:729: factory DartString.cons(DartString first,
DartString second) {
ok, will do.

https://chromiumcodereview.appspot.com/9642001/diff/1/frog/leg/tree/nodes.dar...
frog/leg/tree/nodes.dart:734: DartString();
Isn't that only if I don't have any other constructors, and factories count?

[ngeoffray, 2012-03-08 11:21:21]

https://chromiumcodereview.appspot.com/9642001/diff/1/frog/leg/tree/nodes.dart
File frog/leg/tree/nodes.dart (right):

https://chromiumcodereview.appspot.com/9642001/diff/1/frog/leg/tree/nodes.dar...
frog/leg/tree/nodes.dart:734: DartString();
On 2012/03/08 11:17:49, Lasse Reichstein Nielsen wrote:
> Isn't that only if I don't have any other constructors, and factories count?

Yes, you're right. I got confused by the mix of factory constructors/empty
constructor.

[Lasse Reichstein Nielsen, 2012-03-08 14:55:03]

Any comments, Peter?

[ahe, 2012-03-12 09:31:03]

Hi Lasse,

Before we move further on this, I would like to discuss the design of string
handling in Leg. We have exceeded the amount of complexity I can deal with :-)

Cheers,
Peter

https://chromiumcodereview.appspot.com/9642001/diff/5001/frog/leg/scanner/lis...
File frog/leg/scanner/listener.dart (right):

https://chromiumcodereview.appspot.com/9642001/diff/5001/frog/leg/scanner/lis...
frog/leg/scanner/listener.dart:840: assert(literalCount != 0);
literalCount > 0

https://chromiumcodereview.appspot.com/9642001/diff/5001/frog/leg/scanner/lis...
frog/leg/scanner/listener.dart:842: compiler.internalError("Unnecessary
juxtaposition production");
I don't think there is an instance of the compiler available here. Also, it
should be recoverableError.

https://chromiumcodereview.appspot.com/9642001/diff/5001/frog/leg/scanner/lis...
frog/leg/scanner/listener.dart:844: // A sequence of string literals may contain
string interpolations.
This approach seems really complicated. I do not understand why this has to be
done in the parser.

How many AST nodes do we have for dealing with string literals by now? Why is
one not enough?

https://chromiumcodereview.appspot.com/9642001/diff/5001/frog/leg/tree/nodes....
File frog/leg/tree/nodes.dart (right):

https://chromiumcodereview.appspot.com/9642001/diff/5001/frog/leg/tree/nodes....
frog/leg/tree/nodes.dart:723: class DartString implements Iterable<int> {
What are these classes doing in this file? They are not AST nodes.

Why do we have so many classes for handling strings?

[Lasse Reichstein Nielsen, 2012-03-14 11:40:09]

PTAL.

[ngeoffray, 2012-03-14 12:29:58]

LGTM, with comments

http://codereview.chromium.org/9642001/diff/14001/frog/leg/compile_time_const...
File frog/leg/compile_time_constants.dart (right):

http://codereview.chromium.org/9642001/diff/14001/frog/leg/compile_time_const...
frog/leg/compile_time_constants.dart:772: if (node.dartString !== null) {
Replace with !node.isInterpolation?

http://codereview.chromium.org/9642001/diff/14001/frog/leg/resolver.dart
File frog/leg/resolver.dart (right):

http://codereview.chromium.org/9642001/diff/14001/frog/leg/resolver.dart#newc...
frog/leg/resolver.dart:906: if (node.isInterpolation) node.visitChildren(this);
Do you need the check? I'd prefer if visitChildren handled it.

http://codereview.chromium.org/9642001/diff/14001/frog/leg/ssa/builder.dart
File frog/leg/ssa/builder.dart (right):

http://codereview.chromium.org/9642001/diff/14001/frog/leg/ssa/builder.dart#n...
frog/leg/ssa/builder.dart:1968: if (node.dartString !== null) {
!node.isInterpolation

http://codereview.chromium.org/9642001/diff/14001/frog/leg/ssa/builder.dart#n...
frog/leg/ssa/builder.dart:2577: * TODO(lrn): Consider whether to handle compile
time constant int/boolan
boolan -> boolean

http://codereview.chromium.org/9642001/diff/14001/frog/leg/ssa/builder.dart#n...
frog/leg/ssa/builder.dart:2590: HInstruction prefix = null;
IMO 'current' would be better than 'prefix'. But that's very subjective, because
it *is* a 'prefix'.

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

http://codereview.chromium.org/9642001/diff/14001/frog/leg/ssa/nodes.dart#new...
frog/leg/ssa/nodes.dart:1391: HInstruction fold(HGraph graph) {
Is that dead code? There is no fold method in the instructions anymore, and line
1409 would fail.

http://codereview.chromium.org/9642001/diff/14001/tests/language/language-leg...
File tests/language/language-leg.status (right):

http://codereview.chromium.org/9642001/diff/14001/tests/language/language-leg...
tests/language/language-leg.status:10: AdjacentConstStringLiteralsTest: Pass,
Fail # Not recognizing 'const' constructor call.
So is the Pass,Fail due to an assert? Why not changing the assert to an
internalError?

[Lasse Reichstein Nielsen, 2012-03-14 13:04:48]

http://codereview.chromium.org/9642001/diff/14001/frog/leg/compile_time_const...
File frog/leg/compile_time_constants.dart (right):

http://codereview.chromium.org/9642001/diff/14001/frog/leg/compile_time_const...
frog/leg/compile_time_constants.dart:772: if (node.dartString !== null) {
True.
I'm still not sure I like the isInterpolation, but let's use it until I decide!
:)

http://codereview.chromium.org/9642001/diff/14001/frog/leg/resolver.dart
File frog/leg/resolver.dart (right):

http://codereview.chromium.org/9642001/diff/14001/frog/leg/resolver.dart#newc...
frog/leg/resolver.dart:906: if (node.isInterpolation) node.visitChildren(this);
visitChildren will visit everything - which will be just more juxtapositions and
literals as leaves, so we know already that it's a waste of time.
So, no, we don't need the check, but let's not do unnecessary work if we can
avoid it.

http://codereview.chromium.org/9642001/diff/14001/frog/leg/ssa/builder.dart
File frog/leg/ssa/builder.dart (right):

http://codereview.chromium.org/9642001/diff/14001/frog/leg/ssa/builder.dart#n...
frog/leg/ssa/builder.dart:1968: if (node.dartString !== null) {
Done.
At this point, we know that things are either an interpolation, or the
dartString is non-null.

http://codereview.chromium.org/9642001/diff/14001/frog/leg/ssa/builder.dart#n...
frog/leg/ssa/builder.dart:2577: * TODO(lrn): Consider whether to handle compile
time constant int/boolan
On 2012/03/14 12:29:58, ngeoffray wrote:
> boolan -> boolean

Done.

http://codereview.chromium.org/9642001/diff/14001/frog/leg/ssa/builder.dart#n...
frog/leg/ssa/builder.dart:2590: HInstruction prefix = null;
Yes, it's really an "instruction accumulator" (as opposed to the string
accumulator), but that would just get more confusing.

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

http://codereview.chromium.org/9642001/diff/14001/frog/leg/ssa/nodes.dart#new...
frog/leg/ssa/nodes.dart:1391: HInstruction fold(HGraph graph) {
It's probably dead, just leftover of a merge that didn't recognize it as gone.
I'll remove it and check.

http://codereview.chromium.org/9642001/diff/14001/tests/language/language-leg...
File tests/language/language-leg.status (right):

http://codereview.chromium.org/9642001/diff/14001/tests/language/language-leg...
tests/language/language-leg.status:10: AdjacentConstStringLiteralsTest: Pass,
Fail # Not recognizing 'const' constructor call.
The problem is that we pass, even though we shouldn't.
It tests that  
  static final s = "constant string";
   ...   const SomeClass("$s")
works, which should only be the case if we recognized compile-time constant
interpolations.
But it works because we don't implement "const" but convert it to "new" instead,
so the compile-time constant requirement on the argument isn't checked.

I'll change it to just Pass.

[ahe, 2012-03-15 09:02:56]

I expected to see a change to this line in scanner_task:

      SourceString argument = tag.argument.value.copyWithoutQuotes(1, 1);

I'm having a hard time getting past the changes to tree/nodes.dart. Let's talk
when I get to the office.

http://codereview.chromium.org/9642001/diff/17001/frog/leg/tree/nodes.dart
File frog/leg/tree/nodes.dart (right):

http://codereview.chromium.org/9642001/diff/17001/frog/leg/tree/nodes.dart#ne...
frog/leg/tree/nodes.dart:1090: // A class representing juxtaposed string
literals.
Documentation comment.

http://codereview.chromium.org/9642001/diff/17001/frog/leg/tree/nodes.dart#ne...
frog/leg/tree/nodes.dart:1099: final bool isInterpolation;
My guess: it is redundant! ;-)

http://codereview.chromium.org/9642001/diff/17001/frog/leg/tree/nodes.dart#ne...
frog/leg/tree/nodes.dart:1104: this.dartString = concatenateLiterals(first,
second),
As a matter of principle, doing this much work in a constructor rubs me the
wrong way. It seems like a premature optimization, that as a general pattern is
likely to cause problems if overused.

All the other constructors in this file are simply storing their arguments. This
is by design based on intuition I have built up over years. I'll try to think
more about why I prefer this style, but I'm having a hard time articulating it
right now.

I not convinced that strings deserve special treatment, and I'm worried that
this approach will be adopted in other places.

From the perspective of an IDE, you may want to find the innermost enclosing
expression or statement of the input cursor. For this purpose, the fields
dartString and isInterpolation are unnecessary.

On the other hand, we need this information early in the scanner_task when
reading #-tags. But I would rather have a method on Compiler that given an
Expression returns a String.

http://codereview.chromium.org/9642001/diff/17001/frog/leg/tree/nodes.dart#ne...
frog/leg/tree/nodes.dart:1564: bool visitLiteralString(LiteralString node) =>
false;
Why is this needed?

[Lasse Reichstein Nielsen, 2012-03-19 07:15:32]

Now also adds juxtaposition support to scanner_task.
I gave all the string-nodes a common interface (which, sadly, can't yet extend
Node), and let script_tag contain a StringNode instead of a LiteralString.
We still don't handle interpolations in string-tags.

http://codereview.chromium.org/9642001/diff/17001/frog/leg/tree/nodes.dart
File frog/leg/tree/nodes.dart (right):

http://codereview.chromium.org/9642001/diff/17001/frog/leg/tree/nodes.dart#ne...
frog/leg/tree/nodes.dart:1090: // A class representing juxtaposed string
literals.
On 2012/03/15 09:02:56, ahe wrote:
> Documentation comment.

Done.

http://codereview.chromium.org/9642001/diff/17001/frog/leg/tree/nodes.dart#ne...
frog/leg/tree/nodes.dart:1099: final bool isInterpolation;
Refactored to be not redundant.

http://codereview.chromium.org/9642001/diff/17001/frog/leg/tree/nodes.dart#ne...
frog/leg/tree/nodes.dart:1104: this.dartString = concatenateLiterals(first,
second),
I have made the computation of dartString and isInterpolation dynamic (a getter
that caches the result). This way they won't have any computational overhead.

http://codereview.chromium.org/9642001/diff/17001/frog/leg/tree/nodes.dart#ne...
frog/leg/tree/nodes.dart:1564: bool visitLiteralString(LiteralString node) =>
false;
It's not.
This visitor is build to traverse sting-juxtapositions containing interpolations
and literals, so I thought it was more readable if it handled those explicitly,
but it's not important. I'll remove it (and visitStringInterpolation in the
GetDartStringVisiitor).

[ahe, 2012-03-19 08:13:08]

Initial comments.

https://chromiumcodereview.appspot.com/9642001/diff/19001/frog/leg/tree/nodes...
File frog/leg/tree/nodes.dart (right):

https://chromiumcodereview.appspot.com/9642001/diff/19001/frog/leg/tree/nodes...
frog/leg/tree/nodes.dart:736: interface StringNode /* extends Node  - when
supported */ {
LiteralString currently extends Literal. That's not necessary. Alternatively,
StringNode should extend Literal.

Then I think StringNode can be made a proper class.

[Lasse Reichstein Nielsen, 2012-03-19 09:57:47]

https://chromiumcodereview.appspot.com/9642001/diff/19001/frog/leg/tree/nodes...
File frog/leg/tree/nodes.dart (right):

https://chromiumcodereview.appspot.com/9642001/diff/19001/frog/leg/tree/nodes...
frog/leg/tree/nodes.dart:736: interface StringNode /* extends Node  - when
supported */ {
I'll try.
I'm not sure StringNode makes a good subclass of Literal, since a
StringInterpolation won't have a meaningful "value".

[Lasse Reichstein Nielsen, 2012-03-19 12:10:11]

Made StringNode a proper subclass of Expression, and it Just Worked (TM).

PTAnotherL.

[ahe, 2012-03-19 14:44:20]

Liking you changes to tree/nodes.dart. More comments will follow.

https://chromiumcodereview.appspot.com/9642001/diff/20018/frog/leg/tree/nodes...
File frog/leg/tree/nodes.dart (right):

https://chromiumcodereview.appspot.com/9642001/diff/20018/frog/leg/tree/nodes...
frog/leg/tree/nodes.dart:1138: // Caches the check for whether this
juxtaposition contains a string
Documentation comment.

https://chromiumcodereview.appspot.com/9642001/diff/20018/frog/leg/tree/nodes...
frog/leg/tree/nodes.dart:1140: bool _isInterpolation = null;
I don't like package privacy.

https://chromiumcodereview.appspot.com/9642001/diff/20018/frog/leg/tree/nodes...
frog/leg/tree/nodes.dart:1141: // Caches a Dart string representation of the
entire juxtaposition's
Documentation comment.

https://chromiumcodereview.appspot.com/9642001/diff/20018/frog/leg/tree/nodes...
frog/leg/tree/nodes.dart:1144: DartString _dartString = null;
privacy.

https://chromiumcodereview.appspot.com/9642001/diff/20018/frog/leg/tree/nodes...
frog/leg/tree/nodes.dart:1161: * Should only be called if [isInterpolation]
returns false.
Perhaps you should "assert" that. If the assertion fails, create an exception
that wraps this node, then we can report a nice error somewhere else.

[Lasse Reichstein Nielsen, 2012-03-19 15:07:33]

https://chromiumcodereview.appspot.com/9642001/diff/20018/frog/leg/tree/nodes...
File frog/leg/tree/nodes.dart (right):

https://chromiumcodereview.appspot.com/9642001/diff/20018/frog/leg/tree/nodes...
frog/leg/tree/nodes.dart:1138: // Caches the check for whether this
juxtaposition contains a string
On 2012/03/19 14:44:20, ahe wrote:
> Documentation comment.

Done.

https://chromiumcodereview.appspot.com/9642001/diff/20018/frog/leg/tree/nodes...
frog/leg/tree/nodes.dart:1140: bool _isInterpolation = null;
But it's soooo conventient for making a variable into a getter :)
Will rename to *Cache.

https://chromiumcodereview.appspot.com/9642001/diff/20018/frog/leg/tree/nodes...
frog/leg/tree/nodes.dart:1141: // Caches a Dart string representation of the
entire juxtaposition's
On 2012/03/19 14:44:20, ahe wrote:
> Documentation comment.

Done.

https://chromiumcodereview.appspot.com/9642001/diff/20018/frog/leg/tree/nodes...
frog/leg/tree/nodes.dart:1144: DartString _dartString = null;
On 2012/03/19 14:44:20, ahe wrote:
> privacy.

Done.

https://chromiumcodereview.appspot.com/9642001/diff/20018/frog/leg/tree/nodes...
frog/leg/tree/nodes.dart:1161: * Should only be called if [isInterpolation]
returns false.
On 2012/03/19 14:44:20, ahe wrote:
> Perhaps you should "assert" that. If the assertion fails, create an exception
> that wraps this node, then we can report a nice error somewhere else.

Done.

[ahe, 2012-03-19 15:53:05]

LGTM!!!

Thanks for being tenacious!

Cheers,
Peter

https://chromiumcodereview.appspot.com/9642001/diff/20018/frog/leg/compile_ti...
File frog/leg/compile_time_constants.dart (right):

https://chromiumcodereview.appspot.com/9642001/diff/20018/frog/leg/compile_ti...
frog/leg/compile_time_constants.dart:776: return
super.visitStringJuxtaposition(node);
Cancel directly to produce better error message? AFAIK, a constant can contain
interpolation.

https://chromiumcodereview.appspot.com/9642001/diff/20018/frog/leg/resolver.dart
File frog/leg/resolver.dart (right):

https://chromiumcodereview.appspot.com/9642001/diff/20018/frog/leg/resolver.d...
frog/leg/resolver.dart:921: if (node.isInterpolation) node.visitChildren(this);
Is "if (node.isInterpolation)" a "premature" optimization?

https://chromiumcodereview.appspot.com/9642001/diff/22016/frog/leg/scanner/li...
File frog/leg/scanner/listener.dart (right):

https://chromiumcodereview.appspot.com/9642001/diff/22016/frog/leg/scanner/li...
frog/leg/scanner/listener.dart:516: listener.cancel("String is not a compile
time constant", node: node);
I don't think the message is correct anymore, and I think it is superfluous. You
can always just say:

StringNode node = popNode();

https://chromiumcodereview.appspot.com/9642001/diff/22016/frog/leg/scanner/li...
frog/leg/scanner/listener.dart:522: listener.cancel("String is a
string-interpolation", node: node);
How about: "string interpolation not supported in library tags"

https://chromiumcodereview.appspot.com/9642001/diff/22016/frog/leg/scanner/li...
frog/leg/scanner/listener.dart:861: stringCount -= 1;
Something wrong with --?

https://chromiumcodereview.appspot.com/9642001/diff/22016/frog/leg/scanner/li...
frog/leg/scanner/listener.dart:865: stringCount -= 1;
Ditto.

https://chromiumcodereview.appspot.com/9642001/diff/22016/frog/leg/scanner/sc...
File frog/leg/scanner/scanner_task.dart (right):

https://chromiumcodereview.appspot.com/9642001/diff/22016/frog/leg/scanner/sc...
frog/leg/scanner/scanner_task.dart:27: // TODO(lrn): When supporting
(compile-time constant) interpolations,
This TODO seems confused. We do not support compile-time constants here. We do
support interpolating a few predefined expressions that are not otherwise in
scope.

https://chromiumcodereview.appspot.com/9642001/diff/22016/frog/leg/scanner/sc...
frog/leg/scanner/scanner_task.dart:29: Uri resolved =
base.resolve(argument.dartString.slowToString());
FWIW, I like not having to call copyWithoutQuotes!

https://chromiumcodereview.appspot.com/9642001/diff/22016/frog/leg/scanner/sc...
frog/leg/scanner/scanner_task.dart:110: new
SourceString(tag.prefix.dartString.slowToString());
Something to consider: having a method named asSourceString() which can avoid
copying.

https://chromiumcodereview.appspot.com/9642001/diff/22016/frog/leg/ssa/builde...
File frog/leg/ssa/builder.dart (right):

https://chromiumcodereview.appspot.com/9642001/diff/22016/frog/leg/ssa/builde...
frog/leg/ssa/builder.dart:2722: // Offset used for the synthetic operator token
used by concat.
Documentation comment.

https://chromiumcodereview.appspot.com/9642001/diff/22016/frog/leg/ssa/builde...
frog/leg/ssa/builder.dart:2726: // instead of introducing unnecessary
concatenations.
Ditto.

https://chromiumcodereview.appspot.com/9642001/diff/22016/frog/leg/ssa/builde...
frog/leg/ssa/builder.dart:2728: // The string value generated so far (not
including that which is still
Ditto.

https://chromiumcodereview.appspot.com/9642001/diff/22016/frog/leg/ssa/builde...
frog/leg/ssa/builder.dart:2739: unreachable();
There is no node argument to visitNode, and there is no location given if the
compiler trips over this.

How about:

visitNode(Node node) {
  compiler.internalError('unexpected node', node: node); 
}

https://chromiumcodereview.appspot.com/9642001/diff/22016/frog/leg/tree/nodes...
File frog/leg/tree/nodes.dart (right):

https://chromiumcodereview.appspot.com/9642001/diff/22016/frog/leg/tree/nodes...
frog/leg/tree/nodes.dart:1144: /**
Add newline before comment.

https://chromiumcodereview.appspot.com/9642001/diff/22016/frog/leg/tree/nodes...
frog/leg/tree/nodes.dart:1149: /**
Ditto.

https://chromiumcodereview.appspot.com/9642001/diff/22016/tests/language/lang...
File tests/language/language-leg.status (right):

https://chromiumcodereview.appspot.com/9642001/diff/22016/tests/language/lang...
tests/language/language-leg.status:10: AdjacentConstStringLiteralsTest: Pass #
Not recognizing 'const' constructor call.
Is it passing for the wrong reason?

[Lasse Reichstein Nielsen, 2012-03-20 09:34:33]

https://chromiumcodereview.appspot.com/9642001/diff/20018/frog/leg/compile_ti...
File frog/leg/compile_time_constants.dart (right):

https://chromiumcodereview.appspot.com/9642001/diff/20018/frog/leg/compile_ti...
frog/leg/compile_time_constants.dart:776: return
super.visitStringJuxtaposition(node);
This is just a temporary stopgap. I have another CL that adds support for
compile-time constant interpolations that is waiting for this one to land.

https://chromiumcodereview.appspot.com/9642001/diff/20018/frog/leg/resolver.dart
File frog/leg/resolver.dart (right):

https://chromiumcodereview.appspot.com/9642001/diff/20018/frog/leg/resolver.d...
frog/leg/resolver.dart:921: if (node.isInterpolation) node.visitChildren(this);
Yes.
All optimizations not backed by measurement are premature.
It used to be a safe and quick check, but after the recent change, making
isInterpolation dynamically computed, it's probably not any faster (apart from
computing isInterpolation so it's cached next time it's read).
I'll remove it.

https://chromiumcodereview.appspot.com/9642001/diff/22016/frog/leg/scanner/li...
File frog/leg/scanner/listener.dart (right):

https://chromiumcodereview.appspot.com/9642001/diff/22016/frog/leg/scanner/li...
frog/leg/scanner/listener.dart:516: listener.cancel("String is not a compile
time constant", node: node);
On 2012/03/19 15:53:06, ahe wrote:
> I don't think the message is correct anymore, and I think it is superfluous.
You
> can always just say:
> 
> StringNode node = popNode();

Done.

https://chromiumcodereview.appspot.com/9642001/diff/22016/frog/leg/scanner/li...
frog/leg/scanner/listener.dart:522: listener.cancel("String is a
string-interpolation", node: node);
On 2012/03/19 15:53:06, ahe wrote:
> How about: "string interpolation not supported in library tags"

Done.

https://chromiumcodereview.appspot.com/9642001/diff/22016/frog/leg/scanner/li...
frog/leg/scanner/listener.dart:861: stringCount -= 1;
Only personal taste.
Since x-- is the same as x -= 1, and I prefer to read the latter (it also avoids
the whole "--x" vs "x--" discussion), I tend to use that.
I'll change it to --.

https://chromiumcodereview.appspot.com/9642001/diff/22016/frog/leg/scanner/li...
frog/leg/scanner/listener.dart:865: stringCount -= 1;
On 2012/03/19 15:53:06, ahe wrote:
> Ditto.

Done.

https://chromiumcodereview.appspot.com/9642001/diff/22016/frog/leg/scanner/sc...
File frog/leg/scanner/scanner_task.dart (right):

https://chromiumcodereview.appspot.com/9642001/diff/22016/frog/leg/scanner/sc...
frog/leg/scanner/scanner_task.dart:27: // TODO(lrn): When supporting
(compile-time constant) interpolations,
Reworded.

https://chromiumcodereview.appspot.com/9642001/diff/22016/frog/leg/scanner/sc...
frog/leg/scanner/scanner_task.dart:110: new
SourceString(tag.prefix.dartString.slowToString());
We do have that (dartString.source), but it includes uninterpreted escapes. Is
this what we need here?

Or, if you want quotes too, just use tag.prefix.value.

Personally, I'd just make PrefixElement hold either a StringNode or a DartString
instead of a SourceString. It's an element, so I think DartString would be
appropriate.

https://chromiumcodereview.appspot.com/9642001/diff/22016/frog/leg/ssa/builde...
File frog/leg/ssa/builder.dart (right):

https://chromiumcodereview.appspot.com/9642001/diff/22016/frog/leg/ssa/builde...
frog/leg/ssa/builder.dart:2722: // Offset used for the synthetic operator token
used by concat.
On 2012/03/19 15:53:06, ahe wrote:
> Documentation comment.

Done.

https://chromiumcodereview.appspot.com/9642001/diff/22016/frog/leg/ssa/builde...
frog/leg/ssa/builder.dart:2726: // instead of introducing unnecessary
concatenations.
On 2012/03/19 15:53:06, ahe wrote:
> Ditto.

Done.

https://chromiumcodereview.appspot.com/9642001/diff/22016/frog/leg/ssa/builde...
frog/leg/ssa/builder.dart:2728: // The string value generated so far (not
including that which is still
On 2012/03/19 15:53:06, ahe wrote:
> Ditto.

Done.

https://chromiumcodereview.appspot.com/9642001/diff/22016/frog/leg/ssa/builde...
frog/leg/ssa/builder.dart:2739: unreachable();
Good catch.
It'll most likely never get hit (requires a statement as subexpression of a
StringInterpolation), but better be correct.

https://chromiumcodereview.appspot.com/9642001/diff/22016/frog/leg/tree/nodes...
File frog/leg/tree/nodes.dart (right):

https://chromiumcodereview.appspot.com/9642001/diff/22016/frog/leg/tree/nodes...
frog/leg/tree/nodes.dart:1144: /**
On 2012/03/19 15:53:06, ahe wrote:
> Add newline before comment.

Done.

https://chromiumcodereview.appspot.com/9642001/diff/22016/frog/leg/tree/nodes...
frog/leg/tree/nodes.dart:1149: /**
On 2012/03/19 15:53:06, ahe wrote:
> Ditto.

Done.

https://chromiumcodereview.appspot.com/9642001/diff/22016/tests/language/lang...
File tests/language/language-leg.status (right):

https://chromiumcodereview.appspot.com/9642001/diff/22016/tests/language/lang...
tests/language/language-leg.status:10: AdjacentConstStringLiteralsTest: Pass #
Not recognizing 'const' constructor call.
Yes. It should pass because we supported compile-time constant string
interpolations, but if passes because we don't recognize "const" as requiring a
compile-time constant argument.
It should be fixed to succeede for the right reason in my next CL.

[ahe, 2012-03-20 10:23:01]

FYI

https://chromiumcodereview.appspot.com/9642001/diff/22016/frog/leg/scanner/sc...
File frog/leg/scanner/scanner_task.dart (right):

https://chromiumcodereview.appspot.com/9642001/diff/22016/frog/leg/scanner/sc...
frog/leg/scanner/scanner_task.dart:110: new
SourceString(tag.prefix.dartString.slowToString());
On 2012/03/20 09:34:33, Lasse Reichstein Nielsen wrote:
> We do have that (dartString.source), but it includes uninterpreted escapes. Is
> this what we need here?

Perhaps. I'm not sure what the specification says, but allowing escapes here
doesn't seem right.

> Or, if you want quotes too, just use tag.prefix.value.

I definitely don't want the quotes.

> Personally, I'd just make PrefixElement hold either a StringNode or a
DartString
> instead of a SourceString. It's an element, so I think DartString would be
> appropriate.

I don't see how that would work. The prefix element needs to go into a scope
that is based on SourceStrings. It is a kind of weird identifier.