Issue 8914024: frog: better binding of methods.

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+469 lines, -397 lines)			Patch
M	frog/gen.dart	View	1	5 chunks	+39 lines, -15 lines	0 comments	Download
M	frog/lib/corelib_impl.dart	View		2 chunks	+79 lines, -79 lines	0 comments	Download
M	frog/member.dart	View	1 2	8 chunks	+21 lines, -23 lines	0 comments	Download
M	frog/minfrog	View		172 chunks	+283 lines, -277 lines	0 comments	Download
M	frog/tests/frog/frog.status	View		1 chunk	+1 line, -1 line	0 comments	Download
M	tests/corelib/src/ConstListLiteralTest.dart	View	1 2 3	1 chunk	+8 lines, -0 lines	0 comments	Download
M	tests/language/language.status	View		2 chunks	+0 lines, -2 lines	0 comments	Download
M	tests/language/src/MethodBindingTest.dart	View	1	1 chunk	+11 lines, -0 lines	0 comments	Download
A	tests/language/src/PropertyFieldOverrideTest.dart	View		1 chunk	+27 lines, -0 lines	0 comments	Download

Messages

Total messages: 3 (0 generated)

Expand Messages | Collapse Messages

Jennifer Messerly

lgtm overall. I'd be good to know if there was any performance impact (I suspect ...

9 years ago (2011-12-15 01:13:14 UTC) #2

Siggi Cherem (dart-lang)

9 years ago (2011-12-15 04:35:55 UTC) #3

Thanks!

http://codereview.chromium.org/8914024/diff/5001/frog/gen.dart
File frog/gen.dart (right):

http://codereview.chromium.org/8914024/diff/5001/frog/gen.dart#newcode51
frog/gen.dart:51: if (world.corelib.types['String'].isUsed) {
On 2011/12/15 01:13:14, John Messerly wrote:
> Can you move this annotation to String.split? in other words:
>   List<String> split(Pattern pattern) native { new List<String>(); }

Done. Just checking - is this safe? I thought we still use the 'native' body to
generate code.

http://codereview.chromium.org/8914024/diff/5001/frog/gen.dart#newcode232
frog/gen.dart:232: String _boundMethod(Type type, String name) {
On 2011/12/15 01:13:14, John Messerly wrote:
> nice!
> 
> fwiw, I think you could do all of these with:
> "this.$name.bind(this);"
> 
> or if that doesn't work, it'd be good to understand why

cool, that seems to work. given that it became so simple, I inlined this back
into _maybeGenerateBoundGetter

http://codereview.chromium.org/8914024/diff/5001/frog/gen.dart#newcode487
frog/gen.dart:487: && m.dynamic._providePropertySyntax &&
!m.dynamic._provideFieldSyntax) {
On 2011/12/15 01:13:14, John Messerly wrote:
> perhaps put these on member?

Done.

http://codereview.chromium.org/8914024/diff/5001/frog/gen.dart#newcode881
frog/gen.dart:881: MethodMember m = method;
On 2011/12/15 01:13:14, John Messerly wrote:
> shouldn't need this line anymore

Done.

http://codereview.chromium.org/8914024/diff/5001/frog/gen.dart#newcode887
frog/gen.dart:887: if (m._providePropertySyntax &&
!m.declaringType.isSingletonNative) {
On 2011/12/15 01:13:14, John Messerly wrote:
> why are singleton natives excluded?

nice catch - this was intended to be a temporary check I had while I was working
out a solution in _boundMethod.

http://codereview.chromium.org/8914024/diff/5001/frog/gen.dart#newcode1485
frog/gen.dart:1485: var listLength = listVar.get_(this, 'length', node.list);
On 2011/12/15 01:13:14, John Messerly wrote:
> nice fix. old code was nasty :)

thx.

http://codereview.chromium.org/8914024/diff/5001/frog/member.dart
File frog/member.dart (right):

http://codereview.chromium.org/8914024/diff/5001/frog/member.dart#newcode37
frog/member.dart:37: if (method.definition.body == null && !method.isNative) {
On 2011/12/15 01:13:14, John Messerly wrote:
> what case was this change needed for?

As I add support to bind native methods, I tried to write:

var m = "hi".substring;

that statement would display the error below because substring takes an optional
second argument. Seemed that this check was overly strict in this case.

http://codereview.chromium.org/8914024/diff/5001/frog/member.dart#newcode471
frog/member.dart:471: // TODO(sigmund): should check for this transitively...
On 2011/12/15 01:13:14, John Messerly wrote:
> could you just walk the chain of _overriddenFields?

Currently we don't have a chain (PropertyMember._overridenField is a
FieldMember, which has nothing else above it). We would need to include
properties also (e.g. convert _overridenField into _overridenMember), and add it
also to FieldMembers.

Expand Messages | Collapse Messages