passing and good perf

cleanup resolveMember and get/set

Fixes two get/set tests as a result of cleanup and mostly addresses several other issues to be completed in a follow-up commit.  This version has been better tested and addresses perf and correctness issue in the reverted 922001.

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

[Jennifer Messerly, 2012-01-18 00:03:00]

lgtm

http://codereview.chromium.org/9222004/diff/1/frog/gen.dart
File frog/gen.dart (left):

http://codereview.chromium.org/9222004/diff/1/frog/gen.dart#oldcode2017
frog/gen.dart:2017: // TODO(jmesserly): this needs serious cleanup...
yay, another TODO gone

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

http://codereview.chromium.org/9222004/diff/1/frog/gen.dart#newcode825
frog/gen.dart:825: // TODO(jimhug): Need to do this right - for now we can just
skip freeing.
yeah, I'm with you on this, having tripped over this last week.

We might want to do it per-statement, but I couldn't find an easy way. Maybe
freeing them doesn't actually matter.

http://codereview.chromium.org/9222004/diff/1/frog/gen.dart#newcode1966
frog/gen.dart:1966: isVoid ? ReturnKind.IGNORE : ReturnKind.POST);
nice cleanup

http://codereview.chromium.org/9222004/diff/1/frog/gen.dart#newcode2018
frog/gen.dart:2018: // Very localized optimization for numbers!
(should we have a TODO to make this optimization go away?
Nevermind: I just saw your TODO in Value.)

http://codereview.chromium.org/9222004/diff/1/frog/gen.dart#newcode2020
frog/gen.dart:2020: world.internalError('should not be here', position.span);
I use this pattern too. Almost wonder if we need an "internalAssert" function.
The error messages & print statements are so more useful if they have the span.
(When I'm debugging I often use world.info or .warning instead of print, because
I want the nice source location printout...)

http://codereview.chromium.org/9222004/diff/1/frog/gen.dart#newcode2040
frog/gen.dart:2040: return x.set_(this, name, position, y, kind: kind,
Personally prefer passing these without the named arg, but I can see it either
way.

I was also skipping the spaces (in "[SomeType foo=defaultVal]" and
"foo:theValue"), as in Python style
(http://google-styleguide.googlecode.com/svn/trunk/pyguide.html?showone=Whites...).
But I've noticed that you and Siggi prefer spaces so maybe I was wrong trying to
do that (my natural inclination would've been to have spaces too).

http://codereview.chromium.org/9222004/diff/1/frog/gen.dart#newcode2045
frog/gen.dart:2045: _visitIndexAssign(int kind, IndexExpression xn, Expression
yn,
wow, so much nicer! index assign and do assign finally look like they should :)
They almost don't deserve methods anymore.

http://codereview.chromium.org/9222004/diff/1/frog/lib/corelib_impl.dart
File frog/lib/corelib_impl.dart (right):

http://codereview.chromium.org/9222004/diff/1/frog/lib/corelib_impl.dart#newc...
frog/lib/corelib_impl.dart:409: // Arithmetic operations.
nice!

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

http://codereview.chromium.org/9222004/diff/1/frog/member.dart#newcode189
frog/member.dart:189: // Handles weird interface case.
might be worth expanding this comment :)

as I read it: we have an interface F and member M declared on F. We also have a
class C that is a subtype of I. If we don't find M on C, look for a member on a
supertype of C. We need that because the supertype of C, which we'll call P,
might not be a subtype of I. So we find P.M and add that to our set.

The same problem doesn't happen with classes because they don't have multiple
inheritance.

In this case, couldn't you do "var mem = subtype.getMember(name)" ? I think that
would always work. You'll get duplicates sometimes--but the Set<Member> should
already handle the de-duplication.

(I'm kind of thinking ahead here to the "implicit interface" feature of classes.
Once we have that, we won't want to be checking "isClass" in as many places. I
don't think we really support "implicit interfaces" yet in Frog)

http://codereview.chromium.org/9222004/diff/1/frog/member.dart#newcode1504
frog/member.dart:1504: if (targets.length < 1000) {
nice fix. I like having limits for all of these potentially expensive things.

btw do we actually have some members with 1000 possible targets? :-)

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

http://codereview.chromium.org/9222004/diff/1/frog/minfrog#newcode93
frog/minfrog:93: return this.noSuchMethod$2("_asNonSentinelEntry", []);
It's weird that it's calling noSuchMethod dynamically. Any idea?

http://codereview.chromium.org/9222004/diff/1/frog/minfrog#newcode1311
frog/minfrog:1311: var existingKey = this._keys[hash];
awesome!

http://codereview.chromium.org/9222004/diff/1/frog/type.dart
File frog/type.dart (right):

http://codereview.chromium.org/9222004/diff/1/frog/type.dart#newcode1220
frog/type.dart:1220: // TODO(jimhug): Too much copy-paster with ConcreteType...
looks like you fixed in a follow up change :)

for a while I was trying to pull stuff like this into Type when I could, to
hopefully make it easier to eventually merge them.

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

http://codereview.chromium.org/9222004/diff/1/frog/value.dart#oldcode213
frog/value.dart:213: // TODO(jmesserly): this has gotten ugly again.
it's funny to see a TODO I added Friday get cleaned up by the first day of the
next week :)

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

http://codereview.chromium.org/9222004/diff/1/frog/value.dart#newcode50
frog/value.dart:50: static Value comma(Value x, Value y) {
(comma was inevitable, huh? :) )

http://codereview.chromium.org/9222004/diff/1/frog/value.dart#newcode96
frog/value.dart:96: [bool isDynamic=false, int kind=0, int
returnKind=ReturnKind.IGNORE]) {
oh dear. It's starting to scream for some kind of CallContext isn't it?
(I know ... I started it with isDynamic :) )

http://codereview.chromium.org/9222004/diff/1/frog/value.dart#newcode129
frog/value.dart:129: // TODO(jimhug): Need to support += and noSuchMethod
better.
yeah. It's actually worse than that--I think we are providing the wrong operator
name for all operators & noSuchMethod. Figured we could wait for the mirrors
spec though before fixing it much though.

http://codereview.chromium.org/9222004/diff/1/frog/value.dart#newcode326
frog/value.dart:326: bool _shouldBindDynamically() {
if we aren't testing the REPL anymore, maybe we should kill forceDynamic for
now?

http://codereview.chromium.org/9222004/diff/1/frog/value.dart#newcode330
frog/value.dart:330: // TODO(jimhug): Better type here - currently is
union(Member, MemberSet)
should be able to remove this TODO now