Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10834374:
Add a field to ClassNode that holds a NodeList of its members. (Closed)
|
Created:
8 years, 4 months ago by messick Modified:
8 years, 4 months ago CC:
reviews_dartlang.org, Anton Muhin Visibility:
Public. |
DescriptionAdd a field to ClassNode that holds a NodeList of its members.
Modify Unparser to print the class body.
Add (simple) tests to verify the body is being recorded.
Committed: https://code.google.com/p/dart/source/detail?r=11122
Patch Set 1 #
Total comments: 4
Patch Set 2 : #Patch Set 3 : #Patch Set 4 : #
Total comments: 24
Patch Set 5 : #
Total comments: 4
Patch Set 6 : #
Total comments: 10
Patch Set 7 : #Patch Set 8 : #
Messages
Total messages: 17 (0 generated)
No L*TM as there are no tests. The comments are just nits. You mention a simple test. I don't see it. Please add something to dart/tests/compiler/dart2js/unparser_test.dart. Cheers, Peter https://chromiumcodereview.appspot.com/10834374/diff/1/lib/compiler/implement... File lib/compiler/implementation/tree/nodes.dart (right): https://chromiumcodereview.appspot.com/10834374/diff/1/lib/compiler/implement... lib/compiler/implementation/tree/nodes.dart:210: if (body !== null) body.accept(visitor); // what about typeParameters? Please visit them :-) If you'd rather not, please follow these conventions: * Comments should be a proper sentence (start with an uppercase letter, and end with a period or question mark). * This looks like a TODO, which should be of this form: // TODO(ldap): Comment here. https://chromiumcodereview.appspot.com/10834374/diff/1/lib/compiler/implement... File lib/compiler/implementation/tree/unparser.dart (right): https://chromiumcodereview.appspot.com/10834374/diff/1/lib/compiler/implement... lib/compiler/implementation/tree/unparser.dart:78: for (Node node in body) { Please use the form of for as you see on line 223. The for-in form always allocates an extra object (the iterator), so we avoid it.
And by the way: I'm really excited that you're doing this. It is great to get the IDE perspective on the parser, and these kinds of issues are exactly what I hoped you would uncover.
I had forgotten to include the new test file. I couldn't put the tests in unparse_test.dart because of dependency conflicts. Following the convention for type inference tests I added unparse2_test.dart.
I added code to visit type parameters and added an unparser test for them. I also included the new test file. PTAL https://chromiumcodereview.appspot.com/10834374/diff/1/lib/compiler/implement... File lib/compiler/implementation/tree/nodes.dart (right): https://chromiumcodereview.appspot.com/10834374/diff/1/lib/compiler/implement... lib/compiler/implementation/tree/nodes.dart:210: if (body !== null) body.accept(visitor); // what about typeParameters? On 2012/08/17 06:43:37, ahe wrote: > Please visit them :-) > > If you'd rather not, please follow these conventions: > > * Comments should be a proper sentence (start with an uppercase letter, and end > with a period or question mark). > > * This looks like a TODO, which should be of this form: > > // TODO(ldap): Comment here. Visiting https://chromiumcodereview.appspot.com/10834374/diff/1/lib/compiler/implement... File lib/compiler/implementation/tree/unparser.dart (right): https://chromiumcodereview.appspot.com/10834374/diff/1/lib/compiler/implement... lib/compiler/implementation/tree/unparser.dart:78: for (Node node in body) { On 2012/08/17 06:43:37, ahe wrote: > Please use the form of for as you see on line 223. > > The for-in form always allocates an extra object (the iterator), so we avoid it. Done.
LGTM! https://chromiumcodereview.appspot.com/10834374/diff/5/lib/compiler/implement... File lib/compiler/implementation/tree/nodes.dart (right): https://chromiumcodereview.appspot.com/10834374/diff/5/lib/compiler/implement... lib/compiler/implementation/tree/nodes.dart:210: if (typeParameters != null) typeParameters.accept(visitor); You should probably move this to line 208. Then we visit in order of syntax: class Name<TypeParam> extends Superclass implements Interfaces { body } https://chromiumcodereview.appspot.com/10834374/diff/5/lib/compiler/implement... File lib/compiler/implementation/tree/unparser.dart (right): https://chromiumcodereview.appspot.com/10834374/diff/5/lib/compiler/implement... lib/compiler/implementation/tree/unparser.dart:81: if (!nodes.isEmpty()) { This is probably: nodes.printOn(sb, '\n '); https://chromiumcodereview.appspot.com/10834374/diff/5/tests/compiler/dart2js... File tests/compiler/dart2js/unparser2_test.dart (right): https://chromiumcodereview.appspot.com/10834374/diff/5/tests/compiler/dart2js... tests/compiler/dart2js/unparser2_test.dart:6: #import("../../../lib/compiler/implementation/elements/elements.dart"); // only need CompilationUnitElement If you import parser_helper.dart from this directory, you can probably avoid all these imports. Alternatively, you can create a small library which provides most of doUnparse and MessageCollector. https://chromiumcodereview.appspot.com/10834374/diff/5/tests/compiler/dart2js... tests/compiler/dart2js/unparser2_test.dart:99: Script script = new Script(null,null); Space after comma. https://chromiumcodereview.appspot.com/10834374/diff/5/tests/compiler/dart2js... tests/compiler/dart2js/unparser2_test.dart:107: Node node = listener.popNode(); Consider adding: Expect.isTrue(listener.nodes.isEmpty()); https://chromiumcodereview.appspot.com/10834374/diff/5/tests/compiler/dart2js... tests/compiler/dart2js/unparser2_test.dart:118: throw new Exception(reason); Why are you creating a new Exception? https://chromiumcodereview.appspot.com/10834374/diff/5/tests/compiler/dart2js... tests/compiler/dart2js/unparser2_test.dart:123: void internalErrorOnElement(Element element, String message) { You probably want to say: throw message; Or Expect.fail(message); I prefer the former. https://chromiumcodereview.appspot.com/10834374/diff/5/tests/compiler/dart2js... tests/compiler/dart2js/unparser2_test.dart:127: Element element]) { Ditto.
https://chromiumcodereview.appspot.com/10834374/diff/5/lib/compiler/implement... File lib/compiler/implementation/tree/nodes.dart (right): https://chromiumcodereview.appspot.com/10834374/diff/5/lib/compiler/implement... lib/compiler/implementation/tree/nodes.dart:211: if (body !== null) body.accept(visitor); Body will have all class members, right? To Anton: I guess we don't want to traverse them in collector.
https://chromiumcodereview.appspot.com/10834374/diff/5/lib/compiler/implement... File lib/compiler/implementation/tree/nodes.dart (right): https://chromiumcodereview.appspot.com/10834374/diff/5/lib/compiler/implement... lib/compiler/implementation/tree/nodes.dart:211: if (body !== null) body.accept(visitor); Yes, indeed. Steve, Peter, any opinion on this? We can skip body in our traversing code, maybe with a help of the method like visitChildrenNoBody On 2012/08/21 14:56:34, Roman wrote: > Body will have all class members, right? > > To Anton: I guess we don't want to traverse them in collector.
https://chromiumcodereview.appspot.com/10834374/diff/5/lib/compiler/implement... File lib/compiler/implementation/tree/nodes.dart (right): https://chromiumcodereview.appspot.com/10834374/diff/5/lib/compiler/implement... lib/compiler/implementation/tree/nodes.dart:211: if (body !== null) body.accept(visitor); On 2012/08/21 15:00:16, Anton Muhin wrote: > Yes, indeed. > > Steve, Peter, any opinion on this? We can skip body in our traversing code, > maybe with a help of the method like visitChildrenNoBody > > On 2012/08/21 14:56:34, Roman wrote: > > Body will have all class members, right? > > > > To Anton: I guess we don't want to traverse them in collector. > I think body will be null in your case.
https://chromiumcodereview.appspot.com/10834374/diff/5/lib/compiler/implement... File lib/compiler/implementation/tree/nodes.dart (right): https://chromiumcodereview.appspot.com/10834374/diff/5/lib/compiler/implement... lib/compiler/implementation/tree/nodes.dart:211: if (body !== null) body.accept(visitor); That'd be great, Peter, thanks a lot. On 2012/08/21 15:03:36, ahe wrote: > On 2012/08/21 15:00:16, Anton Muhin wrote: > > Yes, indeed. > > > > Steve, Peter, any opinion on this? We can skip body in our traversing code, > > maybe with a help of the method like visitChildrenNoBody > > > > On 2012/08/21 14:56:34, Roman wrote: > > > Body will have all class members, right? > > > > > > To Anton: I guess we don't want to traverse them in collector. > > > > I think body will be null in your case.
Thanks for all the comments. I've addressed each one, mostly by making the recommended changes. PTAL, especially NodeListener.endClassDeclaration() http://codereview.chromium.org/10834374/diff/5/lib/compiler/implementation/tr... File lib/compiler/implementation/tree/nodes.dart (right): http://codereview.chromium.org/10834374/diff/5/lib/compiler/implementation/tr... lib/compiler/implementation/tree/nodes.dart:210: if (typeParameters != null) typeParameters.accept(visitor); On 2012/08/21 11:56:17, ahe wrote: > You should probably move this to line 208. Then we visit in order of syntax: > > class Name<TypeParam> extends Superclass implements Interfaces { body } Good point. Done. http://codereview.chromium.org/10834374/diff/5/lib/compiler/implementation/tr... lib/compiler/implementation/tree/nodes.dart:211: if (body !== null) body.accept(visitor); On 2012/08/21 15:04:19, Anton Muhin wrote: > That'd be great, Peter, thanks a lot. > > On 2012/08/21 15:03:36, ahe wrote: > > On 2012/08/21 15:00:16, Anton Muhin wrote: > > > Yes, indeed. > > > > > > Steve, Peter, any opinion on this? We can skip body in our traversing code, > > > maybe with a help of the method like visitChildrenNoBody > > > > > > On 2012/08/21 14:56:34, Roman wrote: > > > > Body will have all class members, right? > > > > > > > > To Anton: I guess we don't want to traverse them in collector. > > > > > > > I think body will be null in your case. > Peter, are you sure body will be null? It looks to me like it will be an empty NodeList. I added code to NodeListener.endClassDeclaration() to make sure body is null instead of empty. http://codereview.chromium.org/10834374/diff/5/lib/compiler/implementation/tr... File lib/compiler/implementation/tree/unparser.dart (right): http://codereview.chromium.org/10834374/diff/5/lib/compiler/implementation/tr... lib/compiler/implementation/tree/unparser.dart:81: if (!nodes.isEmpty()) { On 2012/08/21 11:56:17, ahe wrote: > This is probably: > > nodes.printOn(sb, '\n '); That would leave indentation at the end of the last line, which would break the tests. http://codereview.chromium.org/10834374/diff/5/tests/compiler/dart2js/unparse... File tests/compiler/dart2js/unparser2_test.dart (right): http://codereview.chromium.org/10834374/diff/5/tests/compiler/dart2js/unparse... tests/compiler/dart2js/unparser2_test.dart:6: #import("../../../lib/compiler/implementation/elements/elements.dart"); // only need CompilationUnitElement On 2012/08/21 11:56:17, ahe wrote: > If you import parser_helper.dart from this directory, you can probably avoid all > these imports. > > Alternatively, you can create a small library which provides most of doUnparse > and MessageCollector. If I import parser_helper I don't have visibility of DiagnosticListener. Creating a new library sounds like a good idea but I'll leave it for a later CL. It's probably worth revisiting this topic after the new language changes are in place. http://codereview.chromium.org/10834374/diff/5/tests/compiler/dart2js/unparse... tests/compiler/dart2js/unparser2_test.dart:99: Script script = new Script(null,null); On 2012/08/21 11:56:17, ahe wrote: > Space after comma. Done. http://codereview.chromium.org/10834374/diff/5/tests/compiler/dart2js/unparse... tests/compiler/dart2js/unparser2_test.dart:107: Node node = listener.popNode(); On 2012/08/21 11:56:17, ahe wrote: > Consider adding: > > Expect.isTrue(listener.nodes.isEmpty()); Done. http://codereview.chromium.org/10834374/diff/5/tests/compiler/dart2js/unparse... tests/compiler/dart2js/unparser2_test.dart:118: throw new Exception(reason); On 2012/08/21 11:56:17, ahe wrote: > Why are you creating a new Exception? Java hold-over? Changed. http://codereview.chromium.org/10834374/diff/5/tests/compiler/dart2js/unparse... tests/compiler/dart2js/unparser2_test.dart:123: void internalErrorOnElement(Element element, String message) { On 2012/08/21 11:56:17, ahe wrote: > You probably want to say: > > throw message; > > Or > > Expect.fail(message); > > I prefer the former. Done. http://codereview.chromium.org/10834374/diff/5/tests/compiler/dart2js/unparse... tests/compiler/dart2js/unparser2_test.dart:127: Element element]) { On 2012/08/21 11:56:17, ahe wrote: > Ditto. Done.
http://codereview.chromium.org/10834374/diff/5/lib/compiler/implementation/tr... File lib/compiler/implementation/tree/nodes.dart (right): http://codereview.chromium.org/10834374/diff/5/lib/compiler/implementation/tr... lib/compiler/implementation/tree/nodes.dart:211: if (body !== null) body.accept(visitor); On 2012/08/21 15:35:40, messick wrote: > On 2012/08/21 15:04:19, Anton Muhin wrote: > > That'd be great, Peter, thanks a lot. > > > > On 2012/08/21 15:03:36, ahe wrote: > > > On 2012/08/21 15:00:16, Anton Muhin wrote: > > > > Yes, indeed. > > > > > > > > Steve, Peter, any opinion on this? We can skip body in our traversing > code, > > > > maybe with a help of the method like visitChildrenNoBody > > > > > > > > On 2012/08/21 14:56:34, Roman wrote: > > > > > Body will have all class members, right? > > > > > > > > > > To Anton: I guess we don't want to traverse them in collector. > > > > > > > > > > I think body will be null in your case. > > > > Peter, are you sure body will be null? It looks to me like it will be an empty > NodeList. I added code to NodeListener.endClassDeclaration() to make sure body > is null instead of empty. Empty or null, it doesn't matter in this case, I think. The point is that dart2dart never sees anything but an empty body. http://codereview.chromium.org/10834374/diff/5/lib/compiler/implementation/tr... File lib/compiler/implementation/tree/unparser.dart (right): http://codereview.chromium.org/10834374/diff/5/lib/compiler/implementation/tr... lib/compiler/implementation/tree/unparser.dart:81: if (!nodes.isEmpty()) { On 2012/08/21 15:35:40, messick wrote: > On 2012/08/21 11:56:17, ahe wrote: > > This is probably: > > > > nodes.printOn(sb, '\n '); > > That would leave indentation at the end of the last line, which would break the > tests. That would be a bug in printOn. The second argument is called "separatedBy". http://codereview.chromium.org/10834374/diff/9005/lib/compiler/implementation... File lib/compiler/implementation/scanner/listener.dart (right): http://codereview.chromium.org/10834374/diff/9005/lib/compiler/implementation... lib/compiler/implementation/scanner/listener.dart:929: if (body != null && body.isEmpty()) { Why this test? http://codereview.chromium.org/10834374/diff/9005/lib/compiler/implementation... File lib/compiler/implementation/tree/nodes.dart (right): http://codereview.chromium.org/10834374/diff/9005/lib/compiler/implementation... lib/compiler/implementation/tree/nodes.dart:208: if (typeParameters != null) typeParameters.accept(visitor); Use !== for checking null.
Just to be clear. The CL was OK. Now it isn't :-(
PTAL Thanks for catching the missing '='. https://chromiumcodereview.appspot.com/10834374/diff/5/lib/compiler/implement... File lib/compiler/implementation/tree/unparser.dart (right): https://chromiumcodereview.appspot.com/10834374/diff/5/lib/compiler/implement... lib/compiler/implementation/tree/unparser.dart:81: if (!nodes.isEmpty()) { On 2012/08/21 15:43:16, ahe wrote: > On 2012/08/21 15:35:40, messick wrote: > > On 2012/08/21 11:56:17, ahe wrote: > > > This is probably: > > > > > > nodes.printOn(sb, '\n '); > > > > That would leave indentation at the end of the last line, which would break > the > > tests. > > That would be a bug in printOn. The second argument is called "separatedBy". Done. https://chromiumcodereview.appspot.com/10834374/diff/9005/lib/compiler/implem... File lib/compiler/implementation/scanner/listener.dart (right): https://chromiumcodereview.appspot.com/10834374/diff/9005/lib/compiler/implem... lib/compiler/implementation/scanner/listener.dart:929: if (body != null && body.isEmpty()) { On 2012/08/21 15:43:16, ahe wrote: > Why this test? Removed. https://chromiumcodereview.appspot.com/10834374/diff/9005/lib/compiler/implem... File lib/compiler/implementation/tree/nodes.dart (right): https://chromiumcodereview.appspot.com/10834374/diff/9005/lib/compiler/implem... lib/compiler/implementation/tree/nodes.dart:208: if (typeParameters != null) typeParameters.accept(visitor); On 2012/08/21 15:43:16, ahe wrote: > Use !== for checking null. Done.
DBC https://chromiumcodereview.appspot.com/10834374/diff/11004/lib/compiler/imple... File lib/compiler/implementation/tree/unparser.dart (right): https://chromiumcodereview.appspot.com/10834374/diff/11004/lib/compiler/imple... lib/compiler/implementation/tree/unparser.dart:63: if (node.typeParameters != null) { nit: !== https://chromiumcodereview.appspot.com/10834374/diff/11004/lib/compiler/imple... lib/compiler/implementation/tree/unparser.dart:79: if (body != null) { nit: !== https://chromiumcodereview.appspot.com/10834374/diff/11004/tests/compiler/dar... File tests/compiler/dart2js/unparser2_test.dart (right): https://chromiumcodereview.appspot.com/10834374/diff/11004/tests/compiler/dar... tests/compiler/dart2js/unparser2_test.dart:5: #import("../../../lib/compiler/implementation/scanner/scannerlib.dart"); why a new test? cannot it go into unparser? https://chromiumcodereview.appspot.com/10834374/diff/11004/tests/compiler/dar... tests/compiler/dart2js/unparser2_test.dart:19: testClassDef() { up to you, but I would rather assert the strings like class T{} come unchanged when unparsed. https://chromiumcodereview.appspot.com/10834374/diff/11004/tests/compiler/dar... tests/compiler/dart2js/unparser2_test.dart:88: if (lines[j].length > 6) { that looks somewhat fragile to encode a constant like 6
Thanks Anton. I made some changes based on your feedback. https://chromiumcodereview.appspot.com/10834374/diff/11004/lib/compiler/imple... File lib/compiler/implementation/tree/unparser.dart (right): https://chromiumcodereview.appspot.com/10834374/diff/11004/lib/compiler/imple... lib/compiler/implementation/tree/unparser.dart:63: if (node.typeParameters != null) { On 2012/08/21 16:51:32, Anton Muhin wrote: > nit: !== Done. https://chromiumcodereview.appspot.com/10834374/diff/11004/lib/compiler/imple... lib/compiler/implementation/tree/unparser.dart:79: if (body != null) { On 2012/08/21 16:51:32, Anton Muhin wrote: > nit: !== Done. https://chromiumcodereview.appspot.com/10834374/diff/11004/tests/compiler/dar... File tests/compiler/dart2js/unparser2_test.dart (right): https://chromiumcodereview.appspot.com/10834374/diff/11004/tests/compiler/dar... tests/compiler/dart2js/unparser2_test.dart:5: #import("../../../lib/compiler/implementation/scanner/scannerlib.dart"); On 2012/08/21 16:51:32, Anton Muhin wrote: > why a new test? cannot it go into unparser? Peter and I discussed this earlier in the review cycle. https://chromiumcodereview.appspot.com/10834374/diff/11004/tests/compiler/dar... tests/compiler/dart2js/unparser2_test.dart:19: testClassDef() { On 2012/08/21 16:51:32, Anton Muhin wrote: > up to you, but I would rather assert the strings like class T{} come unchanged > when unparsed. At a minimum, it will have a space after the class name. I'm happy to make this example look however most people expect it to look. https://chromiumcodereview.appspot.com/10834374/diff/11004/tests/compiler/dar... tests/compiler/dart2js/unparser2_test.dart:88: if (lines[j].length > 6) { On 2012/08/21 16:51:32, Anton Muhin wrote: > that looks somewhat fragile to encode a constant like 6 OK, I'll define a constant. I didn't bother because this is test code, but it would make it more readable.
LGTM! |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
