Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10449021:
New Prettyprinter class that converts Node-tree to string. (Closed)
|
Created:
8 years, 7 months ago by Roman Modified:
8 years, 6 months ago CC:
reviews_dartlang.org Visibility:
Public. |
DescriptionNew Prettyprinter class that converts Node-tree to string.
BUG=
TEST=
Committed: https://code.google.com/p/dart/source/detail?r=8436
Patch Set 1 #Patch Set 2 : #
Total comments: 6
Patch Set 3 : address comments #Patch Set 4 : Change class name from PrettyPrint to PrettyPrinter #
Total comments: 15
Patch Set 5 : Address Peter's comments. #
Total comments: 5
Patch Set 6 : Address Peter's comments. #
Total comments: 2
Patch Set 7 : Make prettyPrint() method static. #Patch Set 8 : Remove trailing spaces, add NL at EOF #
Messages
Total messages: 14 (0 generated)
Example: for foo.dart with contents:
main() {
a = list[0]
}
the result is:
<FunctionExpression>
. <Modifiers>
. <NodeList/>
. </Modifiers>
. <Identifier token="main"/>
. <NodeList/>
. <Block>
. <NodeList>
. <VariableDefinitions>
. <NodeList>
. . <SendSet>
. . <Identifier token="a"/>
. . <NodeList>
. . <Send>
. . <Send>
. . . <Identifier token="list"/>
. . </Send>
. . <Operator value="[]"/>
. . <NodeList>
. . . <LiteralInt value="0"/>
. . </NodeList>
. . </Send>
. . </NodeList>
. . <Operator value="="/>
. . </SendSet>
. </NodeList>
. </VariableDefinitions>
. </NodeList>
. </Block>
</FunctionExpression>
lgtm w/ comments addressed, but, please, wait for lgtm from Peter. https://chromiumcodereview.appspot.com/10449021/diff/4/lib/compiler/implement... File lib/compiler/implementation/tree/prettyprint.dart (right): https://chromiumcodereview.appspot.com/10449021/diff/4/lib/compiler/implement... lib/compiler/implementation/tree/prettyprint.dart:29: sb.add("<"); do we really want to emulate XML-like syntax? I personally find those brackets distracting, ymmv of course. https://chromiumcodereview.appspot.com/10449021/diff/4/lib/compiler/implement... lib/compiler/implementation/tree/prettyprint.dart:64: depth = 0; please, provide a normal constructor, something like: PrettyPrint() : depth = 0, sb = new StringBuffer(); https://chromiumcodereview.appspot.com/10449021/diff/4/lib/compiler/implement... File lib/compiler/implementation/tree/tree.dart (right): https://chromiumcodereview.appspot.com/10449021/diff/4/lib/compiler/implement... lib/compiler/implementation/tree/tree.dart:15: #source('prettyprint.dart'); I believe we usually try to keep those sorted, Peter, is it correct?
https://chromiumcodereview.appspot.com/10449021/diff/4/lib/compiler/implement... File lib/compiler/implementation/tree/prettyprint.dart (right): https://chromiumcodereview.appspot.com/10449021/diff/4/lib/compiler/implement... lib/compiler/implementation/tree/prettyprint.dart:29: sb.add("<"); On 2012/05/28 06:07:09, antonmuhin wrote: > do we really want to emulate XML-like syntax? I personally find those brackets > distracting, ymmv of course. XML-like syntax is the first thing that comes to my mind when I want to display some tree-like structure. Also it may be convenient to load output XML into browser and close/open branches with a single click (at least in Chrome). https://chromiumcodereview.appspot.com/10449021/diff/4/lib/compiler/implement... lib/compiler/implementation/tree/prettyprint.dart:64: depth = 0; On 2012/05/28 06:07:09, antonmuhin wrote: > please, provide a normal constructor, something like: > > PrettyPrint() : depth = 0, sb = new StringBuffer(); Done. https://chromiumcodereview.appspot.com/10449021/diff/4/lib/compiler/implement... File lib/compiler/implementation/tree/tree.dart (right): https://chromiumcodereview.appspot.com/10449021/diff/4/lib/compiler/implement... lib/compiler/implementation/tree/tree.dart:15: #source('prettyprint.dart'); On 2012/05/28 06:07:09, antonmuhin wrote: > I believe we usually try to keep those sorted, Peter, is it correct? Sorted.
Hi Roman, You have sent a few examples using the output of this class. That has been helpful and convinced me that we should add this class. But if we add this class, then I think we should get rid of the printDebugInfo in Unparser and use PrettyPrinter instead. Cheers, Peter
I changed class name from PrettyPrint to PrettyPrinter. CL that removes printDebugInfo from Unparser is in https://chromiumcodereview.appspot.com/10456080/
Basically good, but I would like some better names for the add...Node methods. I'm thinking something like: openNode/beginNode closeNode/endNode Cheers, Peter https://chromiumcodereview.appspot.com/10449021/diff/8001/lib/compiler/implem... File lib/compiler/implementation/tree/prettyprint.dart (right): https://chromiumcodereview.appspot.com/10449021/diff/8001/lib/compiler/implem... lib/compiler/implementation/tree/prettyprint.dart:21: void addInNode(String type, [Map params]) { What does the method name mean? https://chromiumcodereview.appspot.com/10449021/diff/8001/lib/compiler/implem... lib/compiler/implementation/tree/prettyprint.dart:29: void addInOutNode(String type, [Map params]) { Ditto. https://chromiumcodereview.appspot.com/10449021/diff/8001/lib/compiler/implem... lib/compiler/implementation/tree/prettyprint.dart:36: void addOutNode(String type, [Map params]) { Ditto. https://chromiumcodereview.appspot.com/10449021/diff/8001/lib/compiler/implem... lib/compiler/implementation/tree/prettyprint.dart:47: // TODO(smok): Escape doublequotes in values. When you get to address this TODO, we have code for that, somewhere. https://chromiumcodereview.appspot.com/10449021/diff/8001/lib/compiler/implem... lib/compiler/implementation/tree/prettyprint.dart:55: sb.add(". "); I'd prefer if you didn't add the periods. If you make this proper XML, you can display it in your favorite XML editor or browser. https://chromiumcodereview.appspot.com/10449021/diff/8001/lib/compiler/implem... lib/compiler/implementation/tree/prettyprint.dart:57: sb.add(" "); Perhaps make this a static constant in the beginning of the file. This way it is "easy" to change the indentation level. https://chromiumcodereview.appspot.com/10449021/diff/8001/lib/compiler/implem... lib/compiler/implementation/tree/prettyprint.dart:67: sb = new StringBuffer(); How do you expect to use this method and class? Are you planning on allocating two StringBuffers every time you use it?
Sorry for the delay. https://chromiumcodereview.appspot.com/10449021/diff/8001/lib/compiler/implem... File lib/compiler/implementation/tree/prettyprint.dart (right): https://chromiumcodereview.appspot.com/10449021/diff/8001/lib/compiler/implem... lib/compiler/implementation/tree/prettyprint.dart:21: void addInNode(String type, [Map params]) { On 2012/06/01 12:52:57, ahe wrote: > What does the method name mean? Changed to "openNode" https://chromiumcodereview.appspot.com/10449021/diff/8001/lib/compiler/implem... lib/compiler/implementation/tree/prettyprint.dart:29: void addInOutNode(String type, [Map params]) { On 2012/06/01 12:52:57, ahe wrote: > Ditto. Changed to openAndCloseNode(), not sure if it's the best name. Any suggestions? https://chromiumcodereview.appspot.com/10449021/diff/8001/lib/compiler/implem... lib/compiler/implementation/tree/prettyprint.dart:36: void addOutNode(String type, [Map params]) { On 2012/06/01 12:52:57, ahe wrote: > Ditto. Changed to "closeNode" https://chromiumcodereview.appspot.com/10449021/diff/8001/lib/compiler/implem... lib/compiler/implementation/tree/prettyprint.dart:55: sb.add(". "); On 2012/06/01 12:52:57, ahe wrote: > I'd prefer if you didn't add the periods. If you make this proper XML, you can > display it in your favorite XML editor or browser. Done. https://chromiumcodereview.appspot.com/10449021/diff/8001/lib/compiler/implem... lib/compiler/implementation/tree/prettyprint.dart:57: sb.add(" "); On 2012/06/01 12:52:57, ahe wrote: > Perhaps make this a static constant in the beginning of the file. This way it is > "easy" to change the indentation level. Created static final String _INDENT = " "; Or did you mean const out of the class definition? I would prefer keeping the scope of the constant inside the class. https://chromiumcodereview.appspot.com/10449021/diff/8001/lib/compiler/implem... lib/compiler/implementation/tree/prettyprint.dart:67: sb = new StringBuffer(); On 2012/06/01 12:52:57, ahe wrote: > How do you expect to use this method and class? Are you planning on allocating > two StringBuffers every time you use it? What do you mean by two StringBuffers? I was thinking about this usage: new PrettyPrinter().prettyPrint(node); It may be useful to provide method like this one: void prettyPrintTo(Node node, StringBuffer buffer); to use in Node.toDebugString() for example. Another idea is to provide simple static method: static String prettyPrint(Node node); and private constructor (starting with underscore).
ping, Peter
Adding Nicolas because Peter is too busy with other issues.
LGTM provided you address the comments below. https://chromiumcodereview.appspot.com/10449021/diff/8001/lib/compiler/implem... File lib/compiler/implementation/tree/prettyprint.dart (right): https://chromiumcodereview.appspot.com/10449021/diff/8001/lib/compiler/implem... lib/compiler/implementation/tree/prettyprint.dart:67: sb = new StringBuffer(); The constructor always creates a string buffer. So you end up creating two. Not terrible, but not necessary. On 2012/06/05 09:28:54, Roman wrote: > On 2012/06/01 12:52:57, ahe wrote: > > How do you expect to use this method and class? Are you planning on allocating > > two StringBuffers every time you use it? > > What do you mean by two StringBuffers? > I was thinking about this usage: > new PrettyPrinter().prettyPrint(node); > > It may be useful to provide method like this one: > void prettyPrintTo(Node node, StringBuffer buffer); > > to use in Node.toDebugString() for example. > > Another idea is to provide simple static method: > static String prettyPrint(Node node); > and private constructor (starting with underscore). https://chromiumcodereview.appspot.com/10449021/diff/11001/lib/compiler/imple... File lib/compiler/implementation/tree/prettyprint.dart (right): https://chromiumcodereview.appspot.com/10449021/diff/11001/lib/compiler/imple... lib/compiler/implementation/tree/prettyprint.dart:13: // String used to represent one level of indent. Doc comment. https://chromiumcodereview.appspot.com/10449021/diff/11001/lib/compiler/imple... lib/compiler/implementation/tree/prettyprint.dart:14: static final String _INDENT = " "; This doesn't need to be private, so please don't make private (FYI, I don't think our privacy mechanism works well, and is often misused).
https://chromiumcodereview.appspot.com/10449021/diff/8001/lib/compiler/implem... File lib/compiler/implementation/tree/prettyprint.dart (right): https://chromiumcodereview.appspot.com/10449021/diff/8001/lib/compiler/implem... lib/compiler/implementation/tree/prettyprint.dart:67: sb = new StringBuffer(); On 2012/06/08 07:37:36, ahe wrote: > The constructor always creates a string buffer. So you end up creating two. Not > terrible, but not necessary. > > On 2012/06/05 09:28:54, Roman wrote: > > On 2012/06/01 12:52:57, ahe wrote: > > > How do you expect to use this method and class? Are you planning on > allocating > > > two StringBuffers every time you use it? > > > > What do you mean by two StringBuffers? > > I was thinking about this usage: > > new PrettyPrinter().prettyPrint(node); > > > > It may be useful to provide method like this one: > > void prettyPrintTo(Node node, StringBuffer buffer); > > > > to use in Node.toDebugString() for example. > > > > Another idea is to provide simple static method: > > static String prettyPrint(Node node); > > and private constructor (starting with underscore). > Ah, I see now. Yeah, that's dumb. Changed to just clear the buffer here. https://chromiumcodereview.appspot.com/10449021/diff/11001/lib/compiler/imple... File lib/compiler/implementation/tree/prettyprint.dart (right): https://chromiumcodereview.appspot.com/10449021/diff/11001/lib/compiler/imple... lib/compiler/implementation/tree/prettyprint.dart:13: // String used to represent one level of indent. On 2012/06/08 07:37:36, ahe wrote: > Doc comment. Done. https://chromiumcodereview.appspot.com/10449021/diff/11001/lib/compiler/imple... lib/compiler/implementation/tree/prettyprint.dart:14: static final String _INDENT = " "; On 2012/06/08 07:37:36, ahe wrote: > This doesn't need to be private, so please don't make private (FYI, I don't > think our privacy mechanism works well, and is often misused). Ok, removed underscore. Why do you think it should be public?
https://chromiumcodereview.appspot.com/10449021/diff/11001/lib/compiler/imple... File lib/compiler/implementation/tree/prettyprint.dart (right): https://chromiumcodereview.appspot.com/10449021/diff/11001/lib/compiler/imple... lib/compiler/implementation/tree/prettyprint.dart:14: static final String _INDENT = " "; On 2012/06/08 09:10:15, Roman wrote: > On 2012/06/08 07:37:36, ahe wrote: > > This doesn't need to be private, so please don't make private (FYI, I don't > > think our privacy mechanism works well, and is often misused). > > Ok, removed underscore. Why do you think it should be public? That is the wrong question :-) The interesting questions are: Why do you think it should be private? Can you write a detailed explanation of the pros and cons of using library privacy to demonstrate that you're fully aware of all the implications of using library privacy? In other words, do you understand exactly how the Dart privacy mechanism differs from, for example, default access (aka package privacy) in Java? Examples of problems I'm aware of regarding library privacy: you cannot access private members from test code. Test code will be in a different library. Also, if not being careful, using private members can cause problems if you use implicit interfaces to mock up stuff. Generally, library privacy is hard to use without using some of the benefits we have built in to Dart. Your intuition from Java or C++ does not apply. You should use library privacy much less frequently, and rely on interfaces to hide implementation details.
https://chromiumcodereview.appspot.com/10449021/diff/16001/lib/compiler/imple... File lib/compiler/implementation/tree/prettyprint.dart (right): https://chromiumcodereview.appspot.com/10449021/diff/16001/lib/compiler/imple... lib/compiler/implementation/tree/prettyprint.dart:80: String prettyPrint(Node node) { I'm not really sure I understand the purpose of this method. It is not re-entrant, so why would I want to call prettyPrint twice on the same PrettyPrinter. Why not turn this into a static method: static String prettyPrint(Node node) { var p = new PrettyPrinter(); node.accept(p); return p.sb.toString(); }
https://chromiumcodereview.appspot.com/10449021/diff/16001/lib/compiler/imple... File lib/compiler/implementation/tree/prettyprint.dart (right): https://chromiumcodereview.appspot.com/10449021/diff/16001/lib/compiler/imple... lib/compiler/implementation/tree/prettyprint.dart:80: String prettyPrint(Node node) { On 2012/06/08 09:45:31, ahe wrote: > I'm not really sure I understand the purpose of this method. It is not > re-entrant, so why would I want to call prettyPrint twice on the same > PrettyPrinter. Why not turn this into a static method: > > static String prettyPrint(Node node) { > var p = new PrettyPrinter(); > node.accept(p); > return p.sb.toString(); > } Yes, this is great. Done. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
