cleanup to Value - fix for StringEscapesTest

cleanup to Value - fix for StringEscapesTest
Motivation of this change was cleaning up value.dart.  This also led to finally handling parsing and escaping string literals in a rational way - which accidentally fixed a test.  This is intended to setup a future change that can simplify EvaluatedValue hopefully dramatically.  Changes to await were required due to refactoring of tree.

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

[jimhug, 2012-01-07 03:07:12]

TBR

[Jennifer Messerly, 2012-01-09 20:16:21]

lgtm

http://codereview.chromium.org/9121025/diff/1/frog/member.dart
File frog/member.dart (left):

http://codereview.chromium.org/9121025/diff/1/frog/member.dart#oldcode1333
frog/member.dart:1333: String _normConcat(Value a, Value b) {
yay

http://codereview.chromium.org/9121025/diff/1/frog/member.dart
File frog/member.dart (right):

http://codereview.chromium.org/9121025/diff/1/frog/member.dart#newcode31
frog/member.dart:31: if (definition.value.span.start == definition.span.start) {
I think I originally suggested the property to make it a bit more clear why
we're checking a span. Maybe we could update the comment?

IIRC this check is for the "someMethod([namedArgWithImplicitNullDefault])" case.
But checking the span is rather subtle.

http://codereview.chromium.org/9121025/diff/1/frog/parser.dart
File frog/parser.dart (left):

http://codereview.chromium.org/9121025/diff/1/frog/parser.dart#oldcode72
frog/parser.dart:72: _maybeEat(TokenKind.INCOMPLETE_MULTILINE_STRING_DQ) ||
I think this was REPL support code that had found its way into
isPrematureEndOfFile.

Not sure which cases it was trying to support, though. I'm fine with removing
for now, as we try to simplify the compiler. (one idea I had: REPL could have an
derived parser class or something like that, so it's clear which code is for
normal Dart parsing and which code is for REPL support.)

http://codereview.chromium.org/9121025/diff/1/frog/parser.dart#oldcode1168
frog/parser.dart:1168: // TODO(jmesserly): HACK add a newline to everything
that's not
--hack;

http://codereview.chromium.org/9121025/diff/1/frog/tokenizer.dart
File frog/tokenizer.dart (right):

http://codereview.chromium.org/9121025/diff/1/frog/tokenizer.dart#newcode269
frog/tokenizer.dart:269: if (_source.text[_startIndex + 4] == '\n') {
might be nice to tweak this as:

int start = _startIndex + 4;
if (_source.text[start] == '\n') start++;
s = _source.text.substring(start, _index - 3);

http://codereview.chromium.org/9121025/diff/1/frog/tokenizer.dart#newcode281
frog/tokenizer.dart:281: var buf = new List<int>();
<int>[] ?

http://codereview.chromium.org/9121025/diff/1/frog/tokenizer.dart#newcode349
frog/tokenizer.dart:349: return _makeStringToken(new List<int>(), false);
<int>[] ?

http://codereview.chromium.org/9121025/diff/1/frog/utils.dart
File frog/utils.dart (left):

http://codereview.chromium.org/9121025/diff/1/frog/utils.dart#oldcode82
frog/utils.dart:82: // TODO(jmesserly): it'd be nice to deal with this in the
tokenizer, rather than
yay, another todo--

http://codereview.chromium.org/9121025/diff/1/frog/value.dart
File frog/value.dart (right):

http://codereview.chromium.org/9121025/diff/1/frog/value.dart#newcode532
frog/value.dart:532: // TODO(jimhug): This could be much more efficient
one thing that might help: we could change StringBuffer's impl to just be a
String. I don't think we need it to build the list, because concat is so well
optimized in JS. I've been meaning to do a microbenchmark on at some point :)

http://codereview.chromium.org/9121025/diff/1/frog/value.dart#newcode602
frog/value.dart:602: return canonicalCode;
nice simplification

[jimhug, 2012-01-09 21:19:05]

Thanks and will include suggestions in coming CL.

http://codereview.chromium.org/9121025/diff/1/frog/member.dart
File frog/member.dart (right):

http://codereview.chromium.org/9121025/diff/1/frog/member.dart#newcode31
frog/member.dart:31: if (definition.value.span.start == definition.span.start) {
On 2012/01/09 20:16:26, John Messerly wrote:
> I think I originally suggested the property to make it a bit more clear why
> we're checking a span. Maybe we could update the comment?
> 
> IIRC this check is for the "someMethod([namedArgWithImplicitNullDefault])"
case.
> But checking the span is rather subtle.
I believe that Bob is going to undo this change, so I'll leave this one
untouched.

http://codereview.chromium.org/9121025/diff/1/frog/parser.dart
File frog/parser.dart (left):

http://codereview.chromium.org/9121025/diff/1/frog/parser.dart#oldcode72
frog/parser.dart:72: _maybeEat(TokenKind.INCOMPLETE_MULTILINE_STRING_DQ) ||
This was originally my code intended to handle single-line tokenization by
keeping track of a tokenizer state for multiple lines when needed.  It turned
out to be a bad idea both for the tokenizer (at least based on my tip
experience) and to not really work given the complicated states we potentially
have now with string interpolation and nested comments.


On 2012/01/09 20:16:26, John Messerly wrote:
> I think this was REPL support code that had found its way into
> isPrematureEndOfFile.
> 
> Not sure which cases it was trying to support, though. I'm fine with removing
> for now, as we try to simplify the compiler. (one idea I had: REPL could have
an
> derived parser class or something like that, so it's clear which code is for
> normal Dart parsing and which code is for REPL support.)

http://codereview.chromium.org/9121025/diff/1/frog/tokenizer.dart
File frog/tokenizer.dart (right):

http://codereview.chromium.org/9121025/diff/1/frog/tokenizer.dart#newcode269
frog/tokenizer.dart:269: if (_source.text[_startIndex + 4] == '\n') {
Nice - done.
On 2012/01/09 20:16:26, John Messerly wrote:
> might be nice to tweak this as:
> 
> int start = _startIndex + 4;
> if (_source.text[start] == '\n') start++;
> s = _source.text.substring(start, _index - 3);
>

http://codereview.chromium.org/9121025/diff/1/frog/tokenizer.dart#newcode281
frog/tokenizer.dart:281: var buf = new List<int>();
On 2012/01/09 20:16:26, John Messerly wrote:
> <int>[] ?

Done.

http://codereview.chromium.org/9121025/diff/1/frog/tokenizer.dart#newcode349
frog/tokenizer.dart:349: return _makeStringToken(new List<int>(), false);
On 2012/01/09 20:16:26, John Messerly wrote:
> <int>[] ?

Done.

http://codereview.chromium.org/9121025/diff/1/frog/value.dart
File frog/value.dart (right):

http://codereview.chromium.org/9121025/diff/1/frog/value.dart#newcode532
frog/value.dart:532: // TODO(jimhug): This could be much more efficient
Good thought.  Let me know what you learn!
On 2012/01/09 20:16:26, John Messerly wrote:
> one thing that might help: we could change StringBuffer's impl to just be a
> String. I don't think we need it to build the list, because concat is so well
> optimized in JS. I've been meaning to do a microbenchmark on at some point :)

[Siggi Cherem (dart-lang), 2012-01-10 19:40:03]

lgtm