Fixes 2 tests and a crash

Fixes 2 tests and a crash - while cleaning up EvaluatedValue

Another step towards cleaning up the use of Value.  Greatly reduced use of .dynamic and slowly increasing modularity of code for handling consts by moving it into the constant Values themselves.  Apologies for the ugliness of added parens around ALL numeric constants - but this did simplify several parts of the code while fixing a couple of broken tests.  I'm confident this step back in readability won't last out the weekend (though I'm not telling you which one <smile>).

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

[jimhug, 2012-01-08 00:03:57]

tbr

[Jennifer Messerly, 2012-01-09 19:06:54]

lgtm

http://codereview.chromium.org/9143001/diff/1/frog/gen.dart
File frog/gen.dart (right):

http://codereview.chromium.org/9143001/diff/1/frog/gen.dart#newcode1981
frog/gen.dart:1981: return x.binop(kind, y, this, node);
so nice!

http://codereview.chromium.org/9143001/diff/1/frog/gen.dart#newcode2331
frog/gen.dart:2331: //var listType = node.isConst ? world.immutableListType :
world.listType;
remove?

http://codereview.chromium.org/9143001/diff/1/frog/gen.dart#newcode2339
frog/gen.dart:2339: listType = listType.getOrMakeConcreteType([type]);
correct generic List type, awesome :)

http://codereview.chromium.org/9143001/diff/1/frog/gen.dart#newcode2364
frog/gen.dart:2364: var values = new List<Value>();
perhaps:
    var values = <Value>[];
?

http://codereview.chromium.org/9143001/diff/1/frog/gen.dart#newcode2376
frog/gen.dart:2376: if (node.isConst && (keyType is ParameterType ||
keyType.hasTypeParams)) {
long line

http://codereview.chromium.org/9143001/diff/1/frog/gen.dart#newcode2376
frog/gen.dart:2376: if (node.isConst && (keyType is ParameterType ||
keyType.hasTypeParams)) {
keyType.isUnboundType, or something like that? Where it's defined like:
class Type { ...; bool get isUnboundType() => hasTypeParameters; ... }
class ParameterType { ...; bool get isUnboundType() => true; ... }

or maybe hasTypeParameters should return "true" from ParameterType

http://codereview.chromium.org/9143001/diff/1/frog/gen.dart#newcode2396
frog/gen.dart:2396: var value = visitTypedValue(node.items[i+1], valueType);
nit: spacing on i + 1

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

http://codereview.chromium.org/9143001/diff/1/frog/member.dart#newcode425
frog/member.dart:425: /*else if (target.isConst && isFinal) {
remove? or is this a todo

http://codereview.chromium.org/9143001/diff/1/frog/member.dart#newcode942
frog/member.dart:942: argsCode.add(arg.code);
yay

http://codereview.chromium.org/9143001/diff/1/frog/member.dart#newcode1027
frog/member.dart:1027: // ????
is this a TODO?

http://codereview.chromium.org/9143001/diff/1/frog/member.dart#newcode1045
frog/member.dart:1045: // TODO(jmesserly): factor this better
Not your change, but I noticed that this block of code here is obsolete (logic
is now in corejs.dart). =)

http://codereview.chromium.org/9143001/diff/1/frog/minfrog
File frog/minfrog (right):

http://codereview.chromium.org/9143001/diff/1/frog/minfrog#newcode1198
frog/minfrog:1198: if (this._entries.$index(this._nextValidIndex) == const$2) {
Nice. I think this actually improves readability. However one thing we might
want to consider in the future: instead of generating names like const$num, to
generate a string with a more human readable name.

(I also want to shorten $globals)

http://codereview.chromium.org/9143001/diff/1/frog/value.dart
File frog/value.dart (left):

http://codereview.chromium.org/9143001/diff/1/frog/value.dart#oldcode573
frog/value.dart:573: // TODO(jmesserly): the subtypes of Value require a lot of
type checks and
awesome!

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

http://codereview.chromium.org/9143001/diff/1/frog/value.dart#newcode555
frog/value.dart:555: String get code() => '@@@';
Maybe this should include the type name? e.g. '@@@EvaluatedValue@@@'
Slightly worried that someone will copy this class, and not realize they need to
change the "code" too so == isn't broken. But this is possibly too paranoid :)

http://codereview.chromium.org/9143001/diff/1/frog/value.dart#newcode749
frog/value.dart:749: for (var i=0; i < values.length; i++) {
nit: spacing on i=0;

http://codereview.chromium.org/9143001/diff/1/frog/value.dart#newcode859
frog/value.dart:859: /*
remove?

[jimhug, 2012-01-09 21:34:23]

http://codereview.chromium.org/9143001/diff/1/frog/gen.dart
File frog/gen.dart (right):

http://codereview.chromium.org/9143001/diff/1/frog/gen.dart#newcode2331
frog/gen.dart:2331: //var listType = node.isConst ? world.immutableListType :
world.listType;
On 2012/01/09 19:06:54, John Messerly wrote:
> remove?

Done.

http://codereview.chromium.org/9143001/diff/1/frog/gen.dart#newcode2364
frog/gen.dart:2364: var values = new List<Value>();
On 2012/01/09 19:06:54, John Messerly wrote:
> perhaps:
>     var values = <Value>[];
> ?

Done.

http://codereview.chromium.org/9143001/diff/1/frog/gen.dart#newcode2376
frog/gen.dart:2376: if (node.isConst && (keyType is ParameterType ||
keyType.hasTypeParams)) {
On 2012/01/09 19:06:54, John Messerly wrote:
> long line

Done.

http://codereview.chromium.org/9143001/diff/1/frog/gen.dart#newcode2376
frog/gen.dart:2376: if (node.isConst && (keyType is ParameterType ||
keyType.hasTypeParams)) {
Good idea - for longer-term cleanup.
On 2012/01/09 19:06:54, John Messerly wrote:
> keyType.isUnboundType, or something like that? Where it's defined like:
> class Type { ...; bool get isUnboundType() => hasTypeParameters; ... }
> class ParameterType { ...; bool get isUnboundType() => true; ... }
> 
> or maybe hasTypeParameters should return "true" from ParameterType

http://codereview.chromium.org/9143001/diff/1/frog/gen.dart#newcode2396
frog/gen.dart:2396: var value = visitTypedValue(node.items[i+1], valueType);
On 2012/01/09 19:06:54, John Messerly wrote:
> nit: spacing on i + 1

Done.

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

http://codereview.chromium.org/9143001/diff/1/frog/member.dart#newcode425
frog/member.dart:425: /*else if (target.isConst && isFinal) {
Was a todo - but then I decided on something different and forgot to remove... 
Done.
On 2012/01/09 19:06:54, John Messerly wrote:
> remove? or is this a todo

http://codereview.chromium.org/9143001/diff/1/frog/member.dart#newcode1027
frog/member.dart:1027: // ????
On 2012/01/09 19:06:54, John Messerly wrote:
> is this a TODO?

Done.

http://codereview.chromium.org/9143001/diff/1/frog/member.dart#newcode1045
frog/member.dart:1045: // TODO(jmesserly): factor this better
I'm not sure I understand the ramifications of removing this - please remove it
in a future checkin when convenient.
On 2012/01/09 19:06:54, John Messerly wrote:
> Not your change, but I noticed that this block of code here is obsolete (logic
> is now in corejs.dart). =)

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

http://codereview.chromium.org/9143001/diff/1/frog/value.dart#newcode555
frog/value.dart:555: String get code() => '@@@';
Really, this should just throw - the point is to ensure an error if one of these
is written.  I'll switch to creating an error.

On 2012/01/09 19:06:54, John Messerly wrote:
> Maybe this should include the type name? e.g. '@@@EvaluatedValue@@@'
> Slightly worried that someone will copy this class, and not realize they need
to
> change the "code" too so == isn't broken. But this is possibly too paranoid :)

http://codereview.chromium.org/9143001/diff/1/frog/value.dart#newcode749
frog/value.dart:749: for (var i=0; i < values.length; i++) {
On 2012/01/09 19:06:54, John Messerly wrote:
> nit: spacing on i=0;

Done.

http://codereview.chromium.org/9143001/diff/1/frog/value.dart#newcode859
frog/value.dart:859: /*
On 2012/01/09 19:06:54, John Messerly wrote:
> remove?

Done.