Generate getters and setters dynamically.

Generate getters and setters dynamically.

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

[sra1, 2012-05-08 17:59:42]

https://chromiumcodereview.appspot.com/10310060/diff/2002/lib/compiler/implem...
File lib/compiler/implementation/emitter.dart (right):

https://chromiumcodereview.appspot.com/10310060/diff/2002/lib/compiler/implem...
lib/compiler/implementation/emitter.dart:103: constructor = eval(str);
Should this new new Function too?

[kasperl, 2012-05-09 07:53:15]

First round. Looks pretty good, but I'd like your take on encoding the need for
a getter or setter in the field name instead of in a separate bit bucket.

https://chromiumcodereview.appspot.com/10310060/diff/2002/lib/compiler/implem...
File lib/compiler/implementation/emitter.dart (right):

https://chromiumcodereview.appspot.com/10310060/diff/2002/lib/compiler/implem...
lib/compiler/implementation/emitter.dart:86: for (var i = 0; i < fields.length;
i++) {
Have you thought about encoding the need for a getter or a setter in the field
name instead? That would make it more understandable and still pretty compact.

   ["a=", "b?", "c?=", "d"]

You also don't risk running out of bits.

https://chromiumcodereview.appspot.com/10310060/diff/2002/lib/compiler/implem...
lib/compiler/implementation/emitter.dart:90: body += "this." + fields[i] + " = "
+ fields[i] + ";\\n";
Use field instead of fields[i].

https://chromiumcodereview.appspot.com/10310060/diff/2002/lib/compiler/implem...
lib/compiler/implementation/emitter.dart:357:
compiler.universe.hasSetter(member, compiler);
The indentation seems a bit off here.

https://chromiumcodereview.appspot.com/10310060/diff/2002/lib/compiler/implem...
lib/compiler/implementation/emitter.dart:458: // See, if we can dynamically
create getters and setters.
Remove , after See.

https://chromiumcodereview.appspot.com/10310060/diff/2002/lib/compiler/implem...
lib/compiler/implementation/emitter.dart:470: if ((needsDynamicGetter &&
namer.getterName(library, name) !=
Maybe add a helper on namer for this check (takes library, name, and fieldName
as parameters) to make this check easier to digest.

https://chromiumcodereview.appspot.com/10310060/diff/2002/lib/compiler/implem...
File lib/compiler/implementation/namer.dart (right):

https://chromiumcodereview.appspot.com/10310060/diff/2002/lib/compiler/implem...
lib/compiler/implementation/namer.dart:80: return 'function
$functionName(fieldName) { return "set\$" + fieldName; }';
Too much indentation. Wouldn't it look better with => even if it is on a new
line?

https://chromiumcodereview.appspot.com/10310060/diff/2002/lib/compiler/implem...
File lib/compiler/implementation/native_emitter.dart (right):

https://chromiumcodereview.appspot.com/10310060/diff/2002/lib/compiler/implem...
lib/compiler/implementation/native_emitter.dart:6: final emptyElementSet;
Add a comment here that explains what this is used for. It seems a bit wonky.

[ngeoffray, 2012-05-09 08:50:27]

http://codereview.chromium.org/10310060/diff/2002/lib/compiler/implementation...
File lib/compiler/implementation/emitter.dart (right):

http://codereview.chromium.org/10310060/diff/2002/lib/compiler/implementation...
lib/compiler/implementation/emitter.dart:72: function(cls, superclass, fields,
fieldBitSet, prototype) {
Not sure we need the two following lines. They kind of make it harder to
understand this blob.

http://codereview.chromium.org/10310060/diff/2002/lib/compiler/implementation...
lib/compiler/implementation/emitter.dart:78: prototype = fieldBitSet;
This is starting to be confusing. Maybe add another defineClass method for these
callers?

http://codereview.chromium.org/10310060/diff/2002/lib/compiler/implementation...
lib/compiler/implementation/emitter.dart:393: // fields.
Remove the TODO.

http://codereview.chromium.org/10310060/diff/2002/lib/compiler/implementation...
File lib/compiler/implementation/namer.dart (right):

http://codereview.chromium.org/10310060/diff/2002/lib/compiler/implementation...
lib/compiler/implementation/namer.dart:80: return 'function
$functionName(fieldName) { return "set\$" + fieldName; }';
Why not putting that in the JS blob instead?

http://codereview.chromium.org/10310060/diff/2002/lib/compiler/implementation...
File lib/compiler/implementation/native_emitter.dart (right):

http://codereview.chromium.org/10310060/diff/2002/lib/compiler/implementation...
lib/compiler/implementation/native_emitter.dart:6: final emptyElementSet;
final Set<Element>

[floitsch, 2012-05-09 13:58:35]

PTAL.
As suggested by Kasper I have now encoded the requirement into the field-name.
Getters and setters are now always consistent. Before this CL we could have a
'get$prototype' for a field '$prototype'.
There are no checks to make sure that this doesn't diverge, but there is a
comment in the getter/setter method of the namer, and this is something that
wouldn't go unnoticed.

https://chromiumcodereview.appspot.com/10310060/diff/2002/lib/compiler/implem...
File lib/compiler/implementation/emitter.dart (right):

https://chromiumcodereview.appspot.com/10310060/diff/2002/lib/compiler/implem...
lib/compiler/implementation/emitter.dart:72: function(cls, superclass, fields,
fieldBitSet, prototype) {
On 2012/05/09 08:50:28, ngeoffray wrote:
> Not sure we need the two following lines. They kind of make it harder to
> understand this blob.

inlined.

https://chromiumcodereview.appspot.com/10310060/diff/2002/lib/compiler/implem...
lib/compiler/implementation/emitter.dart:78: prototype = fieldBitSet;
On 2012/05/09 08:50:28, ngeoffray wrote:
> This is starting to be confusing. Maybe add another defineClass method for
these
> callers?

removed.

https://chromiumcodereview.appspot.com/10310060/diff/2002/lib/compiler/implem...
lib/compiler/implementation/emitter.dart:86: for (var i = 0; i < fields.length;
i++) {
On 2012/05/09 07:53:15, kasperl wrote:
> Have you thought about encoding the need for a getter or a setter in the field
> name instead? That would make it more understandable and still pretty compact.
> 
>    ["a=", "b?", "c?=", "d"]
> 
> You also don't risk running out of bits.

Done.

https://chromiumcodereview.appspot.com/10310060/diff/2002/lib/compiler/implem...
lib/compiler/implementation/emitter.dart:90: body += "this." + fields[i] + " = "
+ fields[i] + ";\\n";
On 2012/05/09 07:53:15, kasperl wrote:
> Use field instead of fields[i].

Done.

https://chromiumcodereview.appspot.com/10310060/diff/2002/lib/compiler/implem...
lib/compiler/implementation/emitter.dart:103: constructor = eval(str);
On 2012/05/08 17:59:42, sra1 wrote:
> Should this new new Function too?

I would like it to be a "new Function", but then we would lose the name which is
important for debugging.

https://chromiumcodereview.appspot.com/10310060/diff/2002/lib/compiler/implem...
lib/compiler/implementation/emitter.dart:357:
compiler.universe.hasSetter(member, compiler);
On 2012/05/09 07:53:15, kasperl wrote:
> The indentation seems a bit off here.

Done.

https://chromiumcodereview.appspot.com/10310060/diff/2002/lib/compiler/implem...
lib/compiler/implementation/emitter.dart:393: // fields.
On 2012/05/09 08:50:28, ngeoffray wrote:
> Remove the TODO.

Done.

https://chromiumcodereview.appspot.com/10310060/diff/2002/lib/compiler/implem...
lib/compiler/implementation/emitter.dart:458: // See, if we can dynamically
create getters and setters.
On 2012/05/09 07:53:15, kasperl wrote:
> Remove , after See.

Done.

https://chromiumcodereview.appspot.com/10310060/diff/2002/lib/compiler/implem...
lib/compiler/implementation/emitter.dart:470: if ((needsDynamicGetter &&
namer.getterName(library, name) !=
On 2012/05/09 07:53:15, kasperl wrote:
> Maybe add a helper on namer for this check (takes library, name, and fieldName
> as parameters) to make this check easier to digest.

code removed.

https://chromiumcodereview.appspot.com/10310060/diff/2002/lib/compiler/implem...
File lib/compiler/implementation/namer.dart (right):

https://chromiumcodereview.appspot.com/10310060/diff/2002/lib/compiler/implem...
lib/compiler/implementation/namer.dart:80: return 'function
$functionName(fieldName) { return "set\$" + fieldName; }';
On 2012/05/09 08:50:28, ngeoffray wrote:
> Why not putting that in the JS blob instead?

Done.

https://chromiumcodereview.appspot.com/10310060/diff/2002/lib/compiler/implem...
File lib/compiler/implementation/native_emitter.dart (right):

https://chromiumcodereview.appspot.com/10310060/diff/2002/lib/compiler/implem...
lib/compiler/implementation/native_emitter.dart:6: final emptyElementSet;
On 2012/05/09 08:50:28, ngeoffray wrote:
> final Set<Element>

Done.

https://chromiumcodereview.appspot.com/10310060/diff/2002/lib/compiler/implem...
lib/compiler/implementation/native_emitter.dart:6: final emptyElementSet;
On 2012/05/09 07:53:15, kasperl wrote:
> Add a comment here that explains what this is used for. It seems a bit wonky.

removed.

[kasperl, 2012-05-09 14:07:49]

LGTM.

https://chromiumcodereview.appspot.com/10310060/diff/9004/lib/compiler/implem...
File lib/compiler/implementation/emitter.dart (right):

https://chromiumcodereview.appspot.com/10310060/diff/9004/lib/compiler/implem...
lib/compiler/implementation/emitter.dart:89: case '$GETTER_SUFFIX': needsGetter
= true; break;
Indent cases.

https://chromiumcodereview.appspot.com/10310060/diff/9004/lib/compiler/implem...
lib/compiler/implementation/emitter.dart:90: case '$GETTER_SETTER_SUFFIX':
needsGetter = true; // fallthrough.
fallthrough -> Fall-through

https://chromiumcodereview.appspot.com/10310060/diff/9004/lib/compiler/implem...
lib/compiler/implementation/emitter.dart:395: // Getters and setters for non
native members are generated dynamically.
non-native

[floitsch, 2012-05-09 14:11:19]

https://chromiumcodereview.appspot.com/10310060/diff/9004/lib/compiler/implem...
File lib/compiler/implementation/emitter.dart (right):

https://chromiumcodereview.appspot.com/10310060/diff/9004/lib/compiler/implem...
lib/compiler/implementation/emitter.dart:89: case '$GETTER_SUFFIX': needsGetter
= true; break;
On 2012/05/09 14:07:49, kasperl wrote:
> Indent cases.

Done.

https://chromiumcodereview.appspot.com/10310060/diff/9004/lib/compiler/implem...
lib/compiler/implementation/emitter.dart:90: case '$GETTER_SETTER_SUFFIX':
needsGetter = true; // fallthrough.
On 2012/05/09 14:07:49, kasperl wrote:
> fallthrough -> Fall-through

Done.

https://chromiumcodereview.appspot.com/10310060/diff/9004/lib/compiler/implem...
lib/compiler/implementation/emitter.dart:395: // Getters and setters for non
native members are generated dynamically.
On 2012/05/09 14:07:49, kasperl wrote:
> non-native

forgot to upload latest cosmetic changes patchset.
This line is gone. (The bool has a sufficiently explanatory name).