Get rid of duplication of class node unparsing.

lib/compiler/implementation/dart_backend/emitter.dart

 // Copyright (c) 2012, the Dart project authors.  Please see the AUTHORS file
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
 
 String emitCode(
       Compiler compiler,
       Unparser unparser,
       Map<LibraryElement, String> imports,
       Collection<Element> topLevelElements,
       Map<ClassElement, Collection<Element>> classMembers) {
-  final sb = new StringBuffer();
+  unparser.sb = new StringBuffer();

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

This hack does not look good. That 'sb' field looks like unparser's
implementation details, we should not touch it from outside.

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

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.

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

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]

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 :)
And we have a perfect notion for it: a function :)
yes, or pass my own.
no need in unparse, it's a visit now.
I may need to access StringBuffer itself.
Agree
Exactly.
Again, if you'd like to convert a node into a string, you should use function.
I am not sure locking StringBuffer buys us a lot here.
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();
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?
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]

I see visit() as an implementation detail.
static function is a function too:)
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).
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:23]

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:

   final processedVariableLists = new Set<VariableListElement>();
 
   void outputElement(Element element) {
     if (element is SynthesizedConstructorElement) return;
     if (element.isField()) {
       assert(element is VariableElement);
       // Different VariableElement's may refer to the same VariableListElement.
       // Output this list only once.
       // TODO: only emit used variables.
       VariableElement variableElement = element;
       final variableList = variableElement.variables;
       if (!processedVariableLists.contains(variableList)) {
         processedVariableLists.add(variableList);
-        sb.add(unparser.unparse(variableList.parseNode(compiler)));
+        unparser.visit(variableList.parseNode(compiler));
       }
     } else {
-      sb.add(unparser.unparse(element.parseNode(compiler)));
+      unparser.visit(element.parseNode(compiler));
     }
   }
 
   void outputClass(ClassElement classElement, Collection<Element> members) {
-    ClassNode classNode = classElement.parseNode(compiler);
-    // classElement.beginToken is 'class', 'interface', or 'abstract'.
-    sb.add(classNode.beginToken.slowToString());
-    if (classNode.beginToken.slowToString() == 'abstract') {
-      sb.add(' ');
-      sb.add(classNode.beginToken.next.slowToString());  // 'class'
-    }
-    sb.add(' ');
-    sb.add(unparser.unparse(classNode.name));
-    if (classNode.typeParameters !== null) {
-      sb.add(unparser.unparse(classNode.typeParameters));
-    }
-    if (classNode.extendsKeyword !== null) {
-      sb.add(' ');
-      classNode.extendsKeyword.value.printOn(sb);
-      sb.add(' ');
-      sb.add(unparser.unparse(classNode.superclass));
-    }
-    if (!classNode.interfaces.isEmpty()) {
-      sb.add(' ');
-      sb.add(unparser.unparse(classNode.interfaces));
-    }
-    if (classNode.defaultClause !== null) {
-      sb.add(' default ');
-      sb.add(unparser.unparse(classNode.defaultClause));
-    }
-    sb.add('{');
-    members.forEach((element) {
-      // TODO(smok): Filter out default constructors here.
-      outputElement(element);
+    unparser.emitClassWithBody(classElement.parseNode(compiler), () {
+      members.forEach((element) {
+        // TODO(smok): Filter out default constructors here.
+        outputElement(element);
+      });
     });
-    sb.add('}');
   }
 
   final libraries = compiler.libraries;
   for (final uri in libraries.getKeys()) {
     // Same library element may be a value for different uris as of now
     // e.g., core libraryElement is a value for both keys 'dart:core'
     // and full file name.  Only care about uris with dart scheme.
     if (!uri.startsWith('dart:')) continue;
     final lib = libraries[uri];
     if (imports.containsKey(lib)) {
-      sb.add('#import("$uri", prefix: "${imports[lib]}");');
+      unparser.sb.add('#import("$uri", prefix: "${imports[lib]}");');
     }
   }
 
   for (final element in topLevelElements) {
     if (element is ClassElement) {
       outputClass(element, classMembers[element]);
     } else {
       outputElement(element);
     }
   }
 
-  return sb.toString();
+  return unparser.sb.toString();
 }