Fix some bugs regarding native fields.

Fix some bugs regarding native fields.

BUG=http://dartbug.com/11894, http://dartbug.com/11525
R=ngeoffray@google.com

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

[sra1, 2013-07-19 22:07:57]

https://codereview.chromium.org/19693011/diff/1/dart/sdk/lib/_internal/compil...
File dart/sdk/lib/_internal/compiler/implementation/compiler.dart (right):

https://codereview.chromium.org/19693011/diff/1/dart/sdk/lib/_internal/compil...
dart/sdk/lib/_internal/compiler/implementation/compiler.dart:960: assembledCode
= null; // Signals failure.
Where is the check for 'assembledCode == null'?

Would it not be cleaner for that place to check compiler.compilationFailed?

https://codereview.chromium.org/19693011/diff/1/dart/sdk/lib/_internal/compil...
File dart/sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart
(right):

https://codereview.chromium.org/19693011/diff/1/dart/sdk/lib/_internal/compil...
dart/sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:1748: if
(member.isInstanceMember()) {
When is this false?

My understanding is that member must be a field.
If member can be static, what is the meaning of generating a getter or setter
that uses 'this'?

https://codereview.chromium.org/19693011/diff/1/dart/sdk/lib/_internal/compil...
dart/sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:1774: if
(code == 0) {
This function is getting a bit long.
Maybe break out the computation of 'code'?

https://codereview.chromium.org/19693011/diff/1/dart/sdk/lib/_internal/compil...
dart/sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:3129: //
4. Find all classes needed for rti.
Start at 4?

[ahe, 2013-07-20 12:16:52]

I'll add some more comments to the code before submitting.

https://codereview.chromium.org/19693011/diff/1/dart/sdk/lib/_internal/compil...
File dart/sdk/lib/_internal/compiler/implementation/compiler.dart (right):

https://codereview.chromium.org/19693011/diff/1/dart/sdk/lib/_internal/compil...
dart/sdk/lib/_internal/compiler/implementation/compiler.dart:960: assembledCode
= null; // Signals failure.
On 2013/07/19 22:07:57, sra1 wrote:
> Where is the check for 'assembledCode == null'?
> 
> Would it not be cleaner for that place to check compiler.compilationFailed?

Yes. This requires me to change the API in ../compiler.dart which is beyond the
scope of this CL.

https://codereview.chromium.org/19693011/diff/1/dart/sdk/lib/_internal/compil...
File dart/sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart
(right):

https://codereview.chromium.org/19693011/diff/1/dart/sdk/lib/_internal/compil...
dart/sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:1748: if
(member.isInstanceMember()) {
On 2013/07/19 22:07:57, sra1 wrote:
> When is this false?

For static fields.

> My understanding is that member must be a field.
> If member can be static, what is the meaning of generating a getter or setter
> that uses 'this'?

This creates a "compact field specification" for static fields, and is used by
mirrors to tell which static fields exist. We don't actually create getters and
setters for static fields, so the mirror system need some information, and I
felt it made sense to reuse the existing mechanism for instance fields.

https://codereview.chromium.org/19693011/diff/1/dart/sdk/lib/_internal/compil...
dart/sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:1774: if
(code == 0) {
On 2013/07/19 22:07:57, sra1 wrote:
> This function is getting a bit long.
> Maybe break out the computation of 'code'?

Good idea.

https://codereview.chromium.org/19693011/diff/1/dart/sdk/lib/_internal/compil...
dart/sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:3129: //
4. Find all classes needed for rti.
On 2013/07/19 22:07:57, sra1 wrote:
> Start at 4?

I'm not sure what you mean by that. If this step was the first, I wouldn't be
able to compute the set difference.

I have two sets: classes that are instantiated, and classes that are needed for
RTI.

The classes in RTI, that are not instantiated used to be created as "holders".
Now we use the regular class mechanism to generate them.

HOWEVER, the classes not instantiated must be treated specially because they
should only be empty shells.  Their members have not been compiled, in
particular, their members are not intercepted (which is what is causing the
assertion failure on line 1755). Please see my analysis in CL
https://codereview.chromium.org/14018036/.

[ngeoffray, 2013-07-22 12:04:03]

LGTM

https://codereview.chromium.org/19693011/diff/1/dart/sdk/lib/_internal/compil...
File dart/sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart
(right):

https://codereview.chromium.org/19693011/diff/1/dart/sdk/lib/_internal/compil...
dart/sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:1705:
visitClassFields(classElement, emitStatics,
MAke the following anonymous function a function in the emitter?

https://codereview.chromium.org/19693011/diff/1/dart/sdk/lib/_internal/compil...
dart/sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:3161:
throw 'fisk';
fisk -> fish. Or you could just get rid of this code :-)

[ahe, 2013-07-22 12:31:28]

Committed patchset #2 manually as r25272 (presubmit successful).

[ahe, 2013-07-22 12:32:05]

Thank you, Stephen and Nicolas.

Stephen, if you have additional comments, I'll address them in a follow-up CL.

https://codereview.chromium.org/19693011/diff/1/dart/sdk/lib/_internal/compil...
File dart/sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart
(right):

https://codereview.chromium.org/19693011/diff/1/dart/sdk/lib/_internal/compil...
dart/sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:1705:
visitClassFields(classElement, emitStatics,
On 2013/07/22 12:04:03, ngeoffray wrote:
> MAke the following anonymous function a function in the emitter?

I thought of that, but it would require more parameters.  At least the following
local variables are captured: classIsNative, fieldMetadata, and hasMetadata.

https://codereview.chromium.org/19693011/diff/1/dart/sdk/lib/_internal/compil...
dart/sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:1748: if
(member.isInstanceMember()) {
On 2013/07/20 12:16:52, ahe wrote:
> On 2013/07/19 22:07:57, sra1 wrote:
> > When is this false?
> 
> For static fields.
> 
> > My understanding is that member must be a field.
> > If member can be static, what is the meaning of generating a getter or
setter
> > that uses 'this'?
> 
> This creates a "compact field specification" for static fields, and is used by
> mirrors to tell which static fields exist. We don't actually create getters
and
> setters for static fields, so the mirror system need some information, and I
> felt it made sense to reuse the existing mechanism for instance fields.

This is documented in CL 19676002.

https://codereview.chromium.org/19693011/diff/1/dart/sdk/lib/_internal/compil...
dart/sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:1774: if
(code == 0) {
On 2013/07/20 12:16:52, ahe wrote:
> On 2013/07/19 22:07:57, sra1 wrote:
> > This function is getting a bit long.
> > Maybe break out the computation of 'code'?
> 
> Good idea.

I'll do that in another CL to reduce conflicts with CL 19676002.

https://codereview.chromium.org/19693011/diff/1/dart/sdk/lib/_internal/compil...
dart/sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:3161:
throw 'fisk';
On 2013/07/22 12:04:03, ngeoffray wrote:
> fisk -> fish. Or you could just get rid of this code :-)

This is debug code I overlooked.