Do not generate empty native classes. Fixes issue 6872.

Do not generate empty native classes. Fixes issue 6872.
Committed: https://code.google.com/p/dart/source/detail?r=15256

[ahe, 2012-11-22 12:22:05]

I'm unable to reason about this change.  Perhaps it would be simpler to never
emit a trailing comma.

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

https://codereview.chromium.org/11415114/diff/3004/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:239:
$isolatePropertiesName[cls] = $defineClassName(cls, desc[''] || [], desc);
I suggest that you use string juxtaposition to get some inline comments here.

https://codereview.chromium.org/11415114/diff/3004/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:674: {bool
emitLeadingComma: true}) {
Why is this a named argument now?

https://codereview.chromium.org/11415114/diff/3004/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:850: {bool
emitEndingComma: true}) {
I think it would be better to update all the callers instead.  Ad-hoc optional
arguments doesn't really scale and have causes a lot of maintenance problems in
the compiler so far.

[ngeoffray, 2012-11-22 13:15:18]

Thanks Peter. PTAL. Always avoiding the trailing comma like you suggest requires
quite some refactoring that I'd prefer doing in another CL.

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

https://codereview.chromium.org/11415114/diff/3004/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:239:
$isolatePropertiesName[cls] = $defineClassName(cls, desc[''] || [], desc);
On 2012/11/22 12:22:05, ahe wrote:
> I suggest that you use string juxtaposition to get some inline comments here.

Done.

https://codereview.chromium.org/11415114/diff/3004/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:674: {bool
emitLeadingComma: true}) {
On 2012/11/22 12:22:05, ahe wrote:
> Why is this a named argument now?

It was to make it consistent with the other methods.

https://codereview.chromium.org/11415114/diff/3004/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:850: {bool
emitEndingComma: true}) {
On 2012/11/22 12:22:05, ahe wrote:
> I think it would be better to update all the callers instead.  Ad-hoc optional
> arguments doesn't really scale and have causes a lot of maintenance problems
in
> the compiler so far.

Done.

[sra1, 2012-11-22 13:45:21]

I think this would be cleaner if the various methods returned JS ASTs or lists
of ASTs.  Let the printer deal with the commas.  And it will fix some irritating
indentation bugs.

It also gives us the mechanism for improving the sourcemap data, which today is
not in a good state for the top level definitions.

[ahe, 2012-11-22 13:46:41]

On 2012/11/22 13:45:21, sra1 wrote:
> I think this would be cleaner if the various methods returned JS ASTs or lists
> of ASTs.  Let the printer deal with the commas.  And it will fix some
irritating
> indentation bugs.
> 
> It also gives us the mechanism for improving the sourcemap data, which today
is
> not in a good state for the top level definitions.

Sound like a great approach.

[ngeoffray, 2012-11-22 13:50:51]

On 2012/11/22 13:46:41, ahe wrote:
> On 2012/11/22 13:45:21, sra1 wrote:
> > I think this would be cleaner if the various methods returned JS ASTs or
lists
> > of ASTs.  Let the printer deal with the commas.  And it will fix some
> irritating
> > indentation bugs.
> > 
> > It also gives us the mechanism for improving the sourcemap data, which today
> is
> > not in a good state for the top level definitions.
> 
> Sound like a great approach.

Funny that Florian just suggested that to me before I see your messages :) I
could not agree more, but I don't intend rewriting the emitter to use the JS AST
in the next few weeks.

[ahe, 2012-11-22 13:51:52]

LGTM!

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

https://codereview.chromium.org/11415114/diff/6002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:239: '''/* The
second argument of defineNativeClass is the list of fields,
defineNativeClass?!?

[ngeoffray, 2012-11-22 13:54:17]

Thanks Peter.

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

https://codereview.chromium.org/11415114/diff/6002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:239: '''/* The
second argument of defineNativeClass is the list of fields,
On 2012/11/22 13:51:52, ahe wrote:
> defineNativeClass?!?

:-), changed to defineClassName.

[floitsch, 2012-11-22 13:59:28]

LGTM.

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

https://codereview.chromium.org/11415114/diff/6002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:240: which
might be undefined if there is no field. */'''
If the class doesn't have any declared fields (in the '' property of the
description), then provide an empty list of fields.

?

[ngeoffray, 2012-11-22 14:01:41]

Thanks Florian.

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

https://codereview.chromium.org/11415114/diff/6002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:240: which
might be undefined if there is no field. */'''
On 2012/11/22 13:59:30, floitsch wrote:
> If the class doesn't have any declared fields (in the '' property of the
> description), then provide an empty list of fields.
> 
> ?

Done.