Get rid of duplication of class node unparsing.

Get rid of duplication of class node unparsing.

R=smok@google.com

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

[Roman, 2012-08-31 13:52:15]

https://chromiumcodereview.appspot.com/10916053/diff/1/lib/compiler/implement...
File lib/compiler/implementation/dart_backend/emitter.dart (right):

https://chromiumcodereview.appspot.com/10916053/diff/1/lib/compiler/implement...
lib/compiler/implementation/dart_backend/emitter.dart:11: unparser.sb = new
StringBuffer();
This hack does not look good. That 'sb' field looks like unparser's
implementation details, we should not touch it from outside.

https://chromiumcodereview.appspot.com/10916053/diff/1/lib/compiler/implement...
File lib/compiler/implementation/tree/unparser.dart (right):

https://chromiumcodereview.appspot.com/10916053/diff/1/lib/compiler/implement...
lib/compiler/implementation/tree/unparser.dart:60: emitClassWithBody(ClassNode
node, classBodyEmitter) {
Can you please specify the full signature for 'classBodyEmitter'? Without
comments and without any hints like return type and parameters it is really
difficult to understand how it is used.

'classBodyEmitter' method should actually have StringBuffer parameter, that will
solve the problem in Emitter.

Also for consistency this method should be called 'unparseClassWithBody'

[Anton Muhin, 2012-08-31 14:01:09]

https://chromiumcodereview.appspot.com/10916053/diff/1/lib/compiler/implement...
File lib/compiler/implementation/dart_backend/emitter.dart (right):

https://chromiumcodereview.appspot.com/10916053/diff/1/lib/compiler/implement...
lib/compiler/implementation/dart_backend/emitter.dart:11: unparser.sb = new
StringBuffer();
On 2012/08/31 13:52:15, Roman wrote:
> This hack does not look good. That 'sb' field looks like unparser's
> implementation details, we should not touch it from outside.

Let me disagree.  Rather you original decision to create new StringBuffer
instance in unparse was a hack---unparser should have created StringBufffer in
constructor and allow to unparse as many elements as I want.

https://chromiumcodereview.appspot.com/10916053/diff/1/lib/compiler/implement...
File lib/compiler/implementation/tree/unparser.dart (right):

https://chromiumcodereview.appspot.com/10916053/diff/1/lib/compiler/implement...
lib/compiler/implementation/tree/unparser.dart:60: emitClassWithBody(ClassNode
node, classBodyEmitter) {
If emitClassWithBody(ClassNode node, void classBodyEmitter()) is more readable
for you, I'll do it.

And no, I don't think classBodyEmitter should have StringBuffer parameter as it
introduces a tension if I should use goodies like addToken or my manual
emission.

Renaming to unparseClassWithBody

On 2012/08/31 13:52:15, Roman wrote:
> Can you please specify the full signature for 'classBodyEmitter'? Without
> comments and without any hints like return type and parameters it is really
> difficult to understand how it is used.
> 
> 'classBodyEmitter' method should actually have StringBuffer parameter, that
will
> solve the problem in Emitter.
> 
> Also for consistency this method should be called 'unparseClassWithBody'

[Roman, 2012-08-31 15:39:00]

https://chromiumcodereview.appspot.com/10916053/diff/1/lib/compiler/implement...
File lib/compiler/implementation/dart_backend/emitter.dart (right):

https://chromiumcodereview.appspot.com/10916053/diff/1/lib/compiler/implement...
lib/compiler/implementation/dart_backend/emitter.dart:11: unparser.sb = new
StringBuffer();
On 2012/08/31 14:01:09, Anton Muhin wrote:
> On 2012/08/31 13:52:15, Roman wrote:
> > This hack does not look good. That 'sb' field looks like unparser's
> > implementation details, we should not touch it from outside.
> 
> Let me disagree.  Rather you original decision to create new StringBuffer
> instance in unparse was a hack---unparser should have created StringBufffer in
> constructor and allow to unparse as many elements as I want.

1) Well it was not my decision :) What makes you think it's a hack? With current
implementation you can unparse as many elements as you want, just call unparse
several times and concatenate everything. But if you want you can change that:
 - create a new stringbuffer in constructor
 - make unparse just add to that stringbuffer returning nothing
 - create a getter something like 'result' to return sb.toString()
 - probably introduce a static shortcut method unparsNode(Node) that returns a
string, creating Unparser object internally, because most of the times I guess
unparser is used in this way: "new Unparser().unparse(node);"
Did you mean something like that?

2) Now the Unparser usage is relatively simple and clean - you create Unparser
with a renamer and then just call unparse() on nodes. WIth this direct
stringbuffer use the code is more difficult to understand, you as a client of
Unparser are now aware not only about its constructor and unparse() method, but
also about its internals - StringBuffer. If Dart was a more conservative
language, that StringBuffer field would be declared private, because its clients
should not hack into Unparser internals, the same is true about visit() method.

3) Do you agree that it would be cleaner if you don't touch stringbuffer
directly from outside of Unparser? I think it is very important and that's what
I object to.

4) I don't see any reasons why you must change the way we use unparser (I mean
exactly accessing its internals) if you don't have that output class body
problem. But that problem can be solved in another, less hacky way, with a
change of unparser like in 1) or with emitClassWithBody() accepting a list of
member nodes instead of classBodyEmitter function. Either solution looks better
to me than messing with stringbuffer.

[Anton Muhin, 2012-08-31 15:47:48]

https://chromiumcodereview.appspot.com/10916053/diff/1/lib/compiler/implement...
File lib/compiler/implementation/dart_backend/emitter.dart (right):

https://chromiumcodereview.appspot.com/10916053/diff/1/lib/compiler/implement...
lib/compiler/implementation/dart_backend/emitter.dart:11: unparser.sb = new
StringBuffer();
On 2012/08/31 15:39:00, Roman wrote:
> On 2012/08/31 14:01:09, Anton Muhin wrote:
> > On 2012/08/31 13:52:15, Roman wrote:
> > > This hack does not look good. That 'sb' field looks like unparser's
> > > implementation details, we should not touch it from outside.
> > 
> > Let me disagree.  Rather you original decision to create new StringBuffer
> > instance in unparse was a hack---unparser should have created StringBufffer
in
> > constructor and allow to unparse as many elements as I want.
> 
> 1) Well it was not my decision :) What makes you think it's a hack?

If I meet an object with the following property: unparser = new Unparser();
unparser.<any method except for unparse> and get NPE for free, I think something
is wrong :)

> With current
> implementation you can unparse as many elements as you want, just call unparse
> several times and concatenate everything.

And we have a perfect notion for it: a function :)

> But if you want you can change that:
>  - create a new stringbuffer in constructor

yes, or pass my own.

>  - make unparse just add to that stringbuffer returning nothing

no need in unparse, it's a visit now.

>  - create a getter something like 'result' to return sb.toString()

I may need to access StringBuffer itself.

>  - probably introduce a static shortcut method unparsNode(Node) that returns a
> string, creating Unparser object internally, because most of the times I guess
> unparser is used in this way: "new Unparser().unparse(node);"

Agree

> Did you mean something like that?

Exactly.
> 
> 2) Now the Unparser usage is relatively simple and clean - you create Unparser
> with a renamer and then just call unparse() on nodes. WIth this direct
> stringbuffer use the code is more difficult to understand, you as a client of
> Unparser are now aware not only about its constructor and unparse() method,
but
> also about its internals - StringBuffer.

Again, if you'd like to convert a node into a string, you should use function.

> If Dart was a more conservative
> language, that StringBuffer field would be declared private, because its
clients
> should not hack into Unparser internals, the same is true about visit()
method.

I am not sure locking StringBuffer buys us a lot here.

> 
> 3) Do you agree that it would be cleaner if you don't touch stringbuffer
> directly from outside of Unparser? I think it is very important and that's
what
> I object to.

Ideally, I would create an unparser with my string buffer and then invoke emit
which will populate this string buffer:

final StringBuffer sb = new StringBuffer();
final unparse = new Unparser(sb);
emitCode(unparser, ...);
assembeledCode = sb.toString();

> 
> 4) I don't see any reasons why you must change the way we use unparser (I mean
> exactly accessing its internals) if you don't have that output class body
> problem.

Performance is another important consideration.  Plus after all I want to turn a
collection of nodes into a string, why I should create a lot of StringBuffers?

> But that problem can be solved in another, less hacky way, with a
> change of unparser like in 1) or with emitClassWithBody() accepting a list of
> member nodes instead of classBodyEmitter function. Either solution looks
better
> to me than messing with stringbuffer.

Again, I am 'messing' with string buffer as I cannot configure it properly.

Regarding passing methods, it might be a good idea, but it may require some
duplication of code.

[Roman, 2012-08-31 16:49:55]

https://chromiumcodereview.appspot.com/10916053/diff/1/lib/compiler/implement...
File lib/compiler/implementation/dart_backend/emitter.dart (right):

https://chromiumcodereview.appspot.com/10916053/diff/1/lib/compiler/implement...
lib/compiler/implementation/dart_backend/emitter.dart:11: unparser.sb = new
StringBuffer();
On 2012/08/31 15:47:48, Anton Muhin wrote:
> On 2012/08/31 15:39:00, Roman wrote:
> > On 2012/08/31 14:01:09, Anton Muhin wrote:
> > > On 2012/08/31 13:52:15, Roman wrote:
> > > > This hack does not look good. That 'sb' field looks like unparser's
> > > > implementation details, we should not touch it from outside.
> > > 
> > > Let me disagree.  Rather you original decision to create new StringBuffer
> > > instance in unparse was a hack---unparser should have created
StringBufffer
> in
> > > constructor and allow to unparse as many elements as I want.
> > 
> > 1) Well it was not my decision :) What makes you think it's a hack?
> 
> If I meet an object with the following property: unparser = new Unparser();
> unparser.<any method except for unparse> and get NPE for free, I think
something
> is wrong :)
> 
> > With current
> > implementation you can unparse as many elements as you want, just call
unparse
> > several times and concatenate everything.
> 
> And we have a perfect notion for it: a function :)
> 
> > But if you want you can change that:
> >  - create a new stringbuffer in constructor
> 
> yes, or pass my own.
> 
> >  - make unparse just add to that stringbuffer returning nothing
> 
> no need in unparse, it's a visit now.

I see visit() as an implementation detail.

> 
> >  - create a getter something like 'result' to return sb.toString()
> 
> I may need to access StringBuffer itself.
> 
> >  - probably introduce a static shortcut method unparsNode(Node) that returns
a
> > string, creating Unparser object internally, because most of the times I
guess
> > unparser is used in this way: "new Unparser().unparse(node);"
> 
> Agree
> 
> > Did you mean something like that?
> 
> Exactly.
> > 
> > 2) Now the Unparser usage is relatively simple and clean - you create
Unparser
> > with a renamer and then just call unparse() on nodes. WIth this direct
> > stringbuffer use the code is more difficult to understand, you as a client
of
> > Unparser are now aware not only about its constructor and unparse() method,
> but
> > also about its internals - StringBuffer.
> 
> Again, if you'd like to convert a node into a string, you should use function.

static function is a function too:)

> 
> > If Dart was a more conservative
> > language, that StringBuffer field would be declared private, because its
> clients
> > should not hack into Unparser internals, the same is true about visit()
> method.
> 
> I am not sure locking StringBuffer buys us a lot here.
> 
> > 
> > 3) Do you agree that it would be cleaner if you don't touch stringbuffer
> > directly from outside of Unparser? I think it is very important and that's
> what
> > I object to.
> 
> Ideally, I would create an unparser with my string buffer and then invoke emit
> which will populate this string buffer:
> 
> final StringBuffer sb = new StringBuffer();
> final unparse = new Unparser(sb);
> emitCode(unparser, ...);
> assembeledCode = sb.toString();

This code looks much better to me! only because it does not touch Unparser
internals. I would still prefer to let Unparser create StringBuffer (because I
don't think we will ever need to create Unparser with stringbuffer that's not
empty) and have a getter that returns the result as string (more about this at
the end of my reply).

> 
> > 
> > 4) I don't see any reasons why you must change the way we use unparser (I
mean
> > exactly accessing its internals) if you don't have that output class body
> > problem.
> 
> Performance is another important consideration.  Plus after all I want to turn
a
> collection of nodes into a string, why I should create a lot of StringBuffers?
> 
> > But that problem can be solved in another, less hacky way, with a
> > change of unparser like in 1) or with emitClassWithBody() accepting a list
of
> > member nodes instead of classBodyEmitter function. Either solution looks
> better
> > to me than messing with stringbuffer.
> 
> Again, I am 'messing' with string buffer as I cannot configure it properly.
> 
> Regarding passing methods, it might be a good idea, but it may require some
> duplication of code.


Overall, please understand what I'm looking for - a clean and simple usage
pattern of Unparser by its clients. I really miss private and public modifiers
here, because they allow to clearly say what is for clients and what is
implementation detail. But if we don't have these mechanisms, it does not mean
we should not think about it and abuse the fast that everything is accessible.
I'm trying to make a clear division between Unparser's stuff that should be used
by clients (only constructor and unparse() now, but stringbuffer and visit after
your change) and internal stuff that should not be used by clients (visit,
stringbuffer), that's why I think if you do as in 1) or use your variant, it's
good to have unparse() method even if you can use visit(), and have 'result'
getter even if you can always access stringbuffer directly. This makes usage
pattern clean and do not intersect with Unparser internals: create Unparser(with
or without stringbuffer)... unparse()... get result, as opposed to 'modify
unparser field -> call visit() or unparse() or any other method?', it is more
difficult to formulate. Does this sound reasonable?

[Anton Muhin, 2012-08-31 18:13:22]

https://chromiumcodereview.appspot.com/10916053/diff/1/lib/compiler/implement...
File lib/compiler/implementation/dart_backend/emitter.dart (right):

https://chromiumcodereview.appspot.com/10916053/diff/1/lib/compiler/implement...
lib/compiler/implementation/dart_backend/emitter.dart:11: unparser.sb = new
StringBuffer();
Sure, I agree with you.  I just didn't want to touch Unparser too much in my
original variant.

Let me rework it following results of our dicussion.

On 2012/08/31 16:49:55, Roman wrote:
> On 2012/08/31 15:47:48, Anton Muhin wrote:
> > On 2012/08/31 15:39:00, Roman wrote:
> > > On 2012/08/31 14:01:09, Anton Muhin wrote:
> > > > On 2012/08/31 13:52:15, Roman wrote:
> > > > > This hack does not look good. That 'sb' field looks like unparser's
> > > > > implementation details, we should not touch it from outside.
> > > > 
> > > > Let me disagree.  Rather you original decision to create new
StringBuffer
> > > > instance in unparse was a hack---unparser should have created
> StringBufffer
> > in
> > > > constructor and allow to unparse as many elements as I want.
> > > 
> > > 1) Well it was not my decision :) What makes you think it's a hack?
> > 
> > If I meet an object with the following property: unparser = new Unparser();
> > unparser.<any method except for unparse> and get NPE for free, I think
> something
> > is wrong :)
> > 
> > > With current
> > > implementation you can unparse as many elements as you want, just call
> unparse
> > > several times and concatenate everything.
> > 
> > And we have a perfect notion for it: a function :)
> > 
> > > But if you want you can change that:
> > >  - create a new stringbuffer in constructor
> > 
> > yes, or pass my own.
> > 
> > >  - make unparse just add to that stringbuffer returning nothing
> > 
> > no need in unparse, it's a visit now.
> 
> I see visit() as an implementation detail.
> 
> > 
> > >  - create a getter something like 'result' to return sb.toString()
> > 
> > I may need to access StringBuffer itself.
> > 
> > >  - probably introduce a static shortcut method unparsNode(Node) that
returns
> a
> > > string, creating Unparser object internally, because most of the times I
> guess
> > > unparser is used in this way: "new Unparser().unparse(node);"
> > 
> > Agree
> > 
> > > Did you mean something like that?
> > 
> > Exactly.
> > > 
> > > 2) Now the Unparser usage is relatively simple and clean - you create
> Unparser
> > > with a renamer and then just call unparse() on nodes. WIth this direct
> > > stringbuffer use the code is more difficult to understand, you as a client
> of
> > > Unparser are now aware not only about its constructor and unparse()
method,
> > but
> > > also about its internals - StringBuffer.
> > 
> > Again, if you'd like to convert a node into a string, you should use
function.
> 
> static function is a function too:)
> 
> > 
> > > If Dart was a more conservative
> > > language, that StringBuffer field would be declared private, because its
> > clients
> > > should not hack into Unparser internals, the same is true about visit()
> > method.
> > 
> > I am not sure locking StringBuffer buys us a lot here.
> > 
> > > 
> > > 3) Do you agree that it would be cleaner if you don't touch stringbuffer
> > > directly from outside of Unparser? I think it is very important and that's
> > what
> > > I object to.
> > 
> > Ideally, I would create an unparser with my string buffer and then invoke
emit
> > which will populate this string buffer:
> > 
> > final StringBuffer sb = new StringBuffer();
> > final unparse = new Unparser(sb);
> > emitCode(unparser, ...);
> > assembeledCode = sb.toString();
> 
> This code looks much better to me! only because it does not touch Unparser
> internals. I would still prefer to let Unparser create StringBuffer (because I
> don't think we will ever need to create Unparser with stringbuffer that's not
> empty) and have a getter that returns the result as string (more about this at
> the end of my reply).
> 
> > 
> > > 
> > > 4) I don't see any reasons why you must change the way we use unparser (I
> mean
> > > exactly accessing its internals) if you don't have that output class body
> > > problem.
> > 
> > Performance is another important consideration.  Plus after all I want to
turn
> a
> > collection of nodes into a string, why I should create a lot of
StringBuffers?
> > 
> > > But that problem can be solved in another, less hacky way, with a
> > > change of unparser like in 1) or with emitClassWithBody() accepting a list
> of
> > > member nodes instead of classBodyEmitter function. Either solution looks
> > better
> > > to me than messing with stringbuffer.
> > 
> > Again, I am 'messing' with string buffer as I cannot configure it properly.
> > 
> > Regarding passing methods, it might be a good idea, but it may require some
> > duplication of code.
> 
> 
> Overall, please understand what I'm looking for - a clean and simple usage
> pattern of Unparser by its clients. I really miss private and public modifiers
> here, because they allow to clearly say what is for clients and what is
> implementation detail. But if we don't have these mechanisms, it does not mean
> we should not think about it and abuse the fast that everything is accessible.
> I'm trying to make a clear division between Unparser's stuff that should be
used
> by clients (only constructor and unparse() now, but stringbuffer and visit
after
> your change) and internal stuff that should not be used by clients (visit,
> stringbuffer), that's why I think if you do as in 1) or use your variant, it's
> good to have unparse() method even if you can use visit(), and have 'result'
> getter even if you can always access stringbuffer directly. This makes usage
> pattern clean and do not intersect with Unparser internals: create
Unparser(with
> or without stringbuffer)... unparse()... get result, as opposed to 'modify
> unparser field -> call visit() or unparse() or any other method?', it is more
> difficult to formulate. Does this sound reasonable?

[Anton Muhin, 2012-09-03 09:39:54]

PTAL

[Roman, 2012-09-03 09:43:23]

lgtm

https://chromiumcodereview.appspot.com/10916053/diff/5004/lib/compiler/implem...
File lib/compiler/implementation/tree/unparser.dart (right):

https://chromiumcodereview.appspot.com/10916053/diff/5004/lib/compiler/implem...
lib/compiler/implementation/tree/unparser.dart:69:
unparseClassWithBody(ClassNode node, classBodyEmitter) {
did we agree on 'void classBodyEmitter()' ?

[Anton Muhin, 2012-09-03 09:52:03]

https://chromiumcodereview.appspot.com/10916053/diff/5004/lib/compiler/implem...
File lib/compiler/implementation/tree/unparser.dart (right):

https://chromiumcodereview.appspot.com/10916053/diff/5004/lib/compiler/implem...
lib/compiler/implementation/tree/unparser.dart:69:
unparseClassWithBody(ClassNode node, classBodyEmitter) {
On 2012/09/03 09:43:23, Roman wrote:
> did we agree on 'void classBodyEmitter()' ?

Done.