Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10795064:
dart2dart: Introduce Renamer that is used by renaming unparser (Closed)
|
Created:
8 years, 5 months ago by Roman Modified:
8 years, 2 months ago CC:
reviews_dartlang.org, ahe Visibility:
Public. |
Descriptiondart2dart: Introduce Renamer that is used by renaming unparser
to rename identifiers before printing them out. The renamer
renames top-level elements to avoid conflicts.
Committed: https://code.google.com/p/dart/source/detail?r=9847
Patch Set 1 #
Total comments: 55
Patch Set 2 : #Patch Set 3 : #
Total comments: 15
Patch Set 4 : #Patch Set 5 : #
Total comments: 14
Patch Set 6 : #Patch Set 7 : #Patch Set 8 : #Patch Set 9 : #
Total comments: 4
Messages
Total messages: 17 (0 generated)
https://chromiumcodereview.appspot.com/10795064/diff/1/lib/compiler/implement... File lib/compiler/implementation/dart_backend/backend.dart (right): https://chromiumcodereview.appspot.com/10795064/diff/1/lib/compiler/implement... lib/compiler/implementation/dart_backend/backend.dart:65: && element is! AbstractFieldElement why? https://chromiumcodereview.appspot.com/10795064/diff/1/lib/compiler/implement... File lib/compiler/implementation/dart_backend/emitter.dart (right): https://chromiumcodereview.appspot.com/10795064/diff/1/lib/compiler/implement... lib/compiler/implementation/dart_backend/emitter.dart:12: Renamer renamer; make it final? https://chromiumcodereview.appspot.com/10795064/diff/1/lib/compiler/implement... lib/compiler/implementation/dart_backend/emitter.dart:27: if (classElement.beginToken.slowToString() == 'abstract') { please, a separate change https://chromiumcodereview.appspot.com/10795064/diff/1/lib/compiler/implement... lib/compiler/implementation/dart_backend/emitter.dart:43: classNode.interfaces.nodes.printOn(sb, classNode.interfaces.delimiter); shouldn't you rename those as well? https://chromiumcodereview.appspot.com/10795064/diff/1/lib/compiler/implement... File lib/compiler/implementation/dart_backend/renamer.dart (right): https://chromiumcodereview.appspot.com/10795064/diff/1/lib/compiler/implement... lib/compiler/implementation/dart_backend/renamer.dart:8: interface Renamer { interfaces are going away, let's use just a single class Renamer (merging w/ ConflictingRenamer) and we'll split it back if need be later. https://chromiumcodereview.appspot.com/10795064/diff/1/lib/compiler/implement... lib/compiler/implementation/dart_backend/renamer.dart:29: class ConflictingRenamer implements Renamer { Conflicting sounds weird, what do you try to convey? https://chromiumcodereview.appspot.com/10795064/diff/1/lib/compiler/implement... lib/compiler/implementation/dart_backend/renamer.dart:31: Map<Element, String> renamed; final? https://chromiumcodereview.appspot.com/10795064/diff/1/lib/compiler/implement... lib/compiler/implementation/dart_backend/renamer.dart:32: Set<String> usedTopLevelIdentifiers; final? https://chromiumcodereview.appspot.com/10795064/diff/1/lib/compiler/implement... lib/compiler/implementation/dart_backend/renamer.dart:33: Map<Element, TreeElements> get resolvedElements() => compiler.enqueuer.resolution.resolvedElements; nit: please, move getter below the fields proper and separate with a blank line https://chromiumcodereview.appspot.com/10795064/diff/1/lib/compiler/implement... lib/compiler/implementation/dart_backend/renamer.dart:41: void setContext(Element element) { I don't like this setContext thing, cannot we create a renamer for each context? https://chromiumcodereview.appspot.com/10795064/diff/1/lib/compiler/implement... lib/compiler/implementation/dart_backend/renamer.dart:48: if (contextElements[send] !== null && contextElements[send].isFunction()) { what about top-level variables? https://chromiumcodereview.appspot.com/10795064/diff/1/lib/compiler/implement... lib/compiler/implementation/dart_backend/renamer.dart:56: if (contextElements == null nit: === null (here and below) https://chromiumcodereview.appspot.com/10795064/diff/1/lib/compiler/implement... lib/compiler/implementation/dart_backend/renamer.dart:56: if (contextElements == null how contextElements can be null? https://chromiumcodereview.appspot.com/10795064/diff/1/lib/compiler/implement... lib/compiler/implementation/dart_backend/renamer.dart:58: // We have no info about this type from resolver. when does it happen? https://chromiumcodereview.appspot.com/10795064/diff/1/lib/compiler/implement... lib/compiler/implementation/dart_backend/renamer.dart:68: if (typeAnnotation.typeName is Send what about generic types? https://chromiumcodereview.appspot.com/10795064/diff/1/lib/compiler/implement... lib/compiler/implementation/dart_backend/renamer.dart:69: && typeAnnotation.typeName.receiver.source.slowToString() == type.name.slowToString()) { what's the case when typeName is Send, but receiver.source != type.name? https://chromiumcodereview.appspot.com/10795064/diff/1/lib/compiler/implement... lib/compiler/implementation/dart_backend/renamer.dart:73: String newName = renameType(type); nit: no need in newName local. https://chromiumcodereview.appspot.com/10795064/diff/1/lib/compiler/implement... lib/compiler/implementation/dart_backend/renamer.dart:77: String renameType(Type type) { nit: => renameElement(type.element); https://chromiumcodereview.appspot.com/10795064/diff/1/lib/compiler/implement... lib/compiler/implementation/dart_backend/renamer.dart:86: if (node.token.slowToString() == context.name.slowToString() it looks like you duplicate a work of a resolver: this names equalities should have been resolved, no? https://chromiumcodereview.appspot.com/10795064/diff/1/lib/compiler/implement... lib/compiler/implementation/dart_backend/renamer.dart:94: return null; again, what about top-level fields and privates? https://chromiumcodereview.appspot.com/10795064/diff/1/lib/compiler/implement... File lib/compiler/implementation/dart_backend/renaming_unparser.dart (right): https://chromiumcodereview.appspot.com/10795064/diff/1/lib/compiler/implement... lib/compiler/implementation/dart_backend/renaming_unparser.dart:5: class RenamingUnparser extends Unparser { do we need both RenamingUnparser and Unparser? I don't think so. https://chromiumcodereview.appspot.com/10795064/diff/1/lib/compiler/implement... lib/compiler/implementation/dart_backend/renaming_unparser.dart:11: if (node.selector !== null && node.selector is TypeAnnotation) { no need in !== check, null is TypeAnnotation should be false, if I remember it right. https://chromiumcodereview.appspot.com/10795064/diff/1/lib/compiler/implement... lib/compiler/implementation/dart_backend/renaming_unparser.dart:12: super.visitSend(node); return super.visitSend(node)? https://chromiumcodereview.appspot.com/10795064/diff/1/tests/compiler/dart2js... File tests/compiler/dart2js/unparser_test.dart (right): https://chromiumcodereview.appspot.com/10795064/diff/1/tests/compiler/dart2js... tests/compiler/dart2js/unparser_test.dart:70: fileUri(path) => new Uri(scheme: 'file', path: path); looks like a lot of code duplication w/ testDart2Dart, rework? https://chromiumcodereview.appspot.com/10795064/diff/1/tests/compiler/dart2js... tests/compiler/dart2js/unparser_test.dart:251: mylib.A getA() { nit: => syntax, please
https://chromiumcodereview.appspot.com/10795064/diff/1/lib/compiler/implement... File lib/compiler/implementation/dart_backend/backend.dart (right): https://chromiumcodereview.appspot.com/10795064/diff/1/lib/compiler/implement... lib/compiler/implementation/dart_backend/backend.dart:65: && element is! AbstractFieldElement On 2012/07/23 11:23:10, Anton Mukhin wrote: > why? I don't remember! I checked again without this constraint and tests pass, dart2js and unparser_test unparse fine. Removed. https://chromiumcodereview.appspot.com/10795064/diff/1/lib/compiler/implement... File lib/compiler/implementation/dart_backend/emitter.dart (right): https://chromiumcodereview.appspot.com/10795064/diff/1/lib/compiler/implement... lib/compiler/implementation/dart_backend/emitter.dart:12: Renamer renamer; On 2012/07/23 11:23:10, Anton Mukhin wrote: > make it final? Done. https://chromiumcodereview.appspot.com/10795064/diff/1/lib/compiler/implement... lib/compiler/implementation/dart_backend/emitter.dart:27: if (classElement.beginToken.slowToString() == 'abstract') { On 2012/07/23 11:23:10, Anton Mukhin wrote: > please, a separate change Done. https://chromiumcodereview.appspot.com/10795064/diff/1/lib/compiler/implement... lib/compiler/implementation/dart_backend/emitter.dart:43: classNode.interfaces.nodes.printOn(sb, classNode.interfaces.delimiter); On 2012/07/23 11:23:10, Anton Mukhin wrote: > shouldn't you rename those as well? Done. https://chromiumcodereview.appspot.com/10795064/diff/1/lib/compiler/implement... File lib/compiler/implementation/dart_backend/renamer.dart (right): https://chromiumcodereview.appspot.com/10795064/diff/1/lib/compiler/implement... lib/compiler/implementation/dart_backend/renamer.dart:8: interface Renamer { On 2012/07/23 11:23:10, Anton Mukhin wrote: > interfaces are going away, let's use just a single class Renamer (merging w/ > ConflictingRenamer) and we'll split it back if need be later. As discussed, created Renamer interface and NoRenamer to use in Node class without any additional deps. https://chromiumcodereview.appspot.com/10795064/diff/1/lib/compiler/implement... lib/compiler/implementation/dart_backend/renamer.dart:29: class ConflictingRenamer implements Renamer { On 2012/07/23 11:23:10, Anton Mukhin wrote: > Conflicting sounds weird, what do you try to convey? That it renames only conflicting elements. I'm happy to change it to something that sounds better. https://chromiumcodereview.appspot.com/10795064/diff/1/lib/compiler/implement... lib/compiler/implementation/dart_backend/renamer.dart:31: Map<Element, String> renamed; On 2012/07/23 11:23:10, Anton Mukhin wrote: > final? Done. https://chromiumcodereview.appspot.com/10795064/diff/1/lib/compiler/implement... lib/compiler/implementation/dart_backend/renamer.dart:32: Set<String> usedTopLevelIdentifiers; On 2012/07/23 11:23:10, Anton Mukhin wrote: > final? Done. https://chromiumcodereview.appspot.com/10795064/diff/1/lib/compiler/implement... lib/compiler/implementation/dart_backend/renamer.dart:33: Map<Element, TreeElements> get resolvedElements() => compiler.enqueuer.resolution.resolvedElements; On 2012/07/23 11:23:10, Anton Mukhin wrote: > nit: please, move getter below the fields proper and separate with a blank line Done. https://chromiumcodereview.appspot.com/10795064/diff/1/lib/compiler/implement... lib/compiler/implementation/dart_backend/renamer.dart:41: void setContext(Element element) { On 2012/07/23 11:23:10, Anton Mukhin wrote: > I don't like this setContext thing, cannot we create a renamer for each context? Unfortunately not. Renamer should have a state (like already renamed elements) that lives as long as we're emitting dart code. Another possible solution is to pass already renamed elements to Renamer constructor, but I don't like this because this is about taking Renamer internals to outside. https://chromiumcodereview.appspot.com/10795064/diff/1/lib/compiler/implement... lib/compiler/implementation/dart_backend/renamer.dart:48: if (contextElements[send] !== null && contextElements[send].isFunction()) { On 2012/07/23 11:23:10, Anton Mukhin wrote: > what about top-level variables? I don't quite understand. What about them? https://chromiumcodereview.appspot.com/10795064/diff/1/lib/compiler/implement... lib/compiler/implementation/dart_backend/renamer.dart:56: if (contextElements == null On 2012/07/23 11:23:10, Anton Mukhin wrote: > nit: === null (here and below) Done. https://chromiumcodereview.appspot.com/10795064/diff/1/lib/compiler/implement... lib/compiler/implementation/dart_backend/renamer.dart:56: if (contextElements == null On 2012/07/23 11:23:10, Anton Mukhin wrote: > how contextElements can be null? this happens for class member fields. https://chromiumcodereview.appspot.com/10795064/diff/1/lib/compiler/implement... lib/compiler/implementation/dart_backend/renamer.dart:58: // We have no info about this type from resolver. On 2012/07/23 11:23:10, Anton Mukhin wrote: > when does it happen? this happens for class member fields. Maybe it's a resolver problem, I added TODO. https://chromiumcodereview.appspot.com/10795064/diff/1/lib/compiler/implement... lib/compiler/implementation/dart_backend/renamer.dart:68: if (typeAnnotation.typeName is Send On 2012/07/23 11:23:10, Anton Mukhin wrote: > what about generic types? Generics are in type arguments. Here we care only about the "core" type. https://chromiumcodereview.appspot.com/10795064/diff/1/lib/compiler/implement... lib/compiler/implementation/dart_backend/renamer.dart:69: && typeAnnotation.typeName.receiver.source.slowToString() == type.name.slowToString()) { On 2012/07/23 11:23:10, Anton Mukhin wrote: > what's the case when typeName is Send, but receiver.source != type.name? For example when we have a method that returns "mylib.A" https://chromiumcodereview.appspot.com/10795064/diff/1/lib/compiler/implement... lib/compiler/implementation/dart_backend/renamer.dart:73: String newName = renameType(type); On 2012/07/23 11:23:10, Anton Mukhin wrote: > nit: no need in newName local. Done. https://chromiumcodereview.appspot.com/10795064/diff/1/lib/compiler/implement... lib/compiler/implementation/dart_backend/renamer.dart:77: String renameType(Type type) { On 2012/07/23 11:23:10, Anton Mukhin wrote: > nit: => renameElement(type.element); Done. https://chromiumcodereview.appspot.com/10795064/diff/1/lib/compiler/implement... lib/compiler/implementation/dart_backend/renamer.dart:86: if (node.token.slowToString() == context.name.slowToString() On 2012/07/23 11:23:10, Anton Mukhin wrote: > it looks like you duplicate a work of a resolver: this names equalities should > have been resolved, no? Added TODO as above. I'll try to investigate if resolver can help us here as my next task. https://chromiumcodereview.appspot.com/10795064/diff/1/lib/compiler/implement... lib/compiler/implementation/dart_backend/renamer.dart:94: return null; On 2012/07/23 11:23:10, Anton Mukhin wrote: > again, what about top-level fields and privates? Again please elaborate :) https://chromiumcodereview.appspot.com/10795064/diff/1/lib/compiler/implement... File lib/compiler/implementation/dart_backend/renaming_unparser.dart (right): https://chromiumcodereview.appspot.com/10795064/diff/1/lib/compiler/implement... lib/compiler/implementation/dart_backend/renaming_unparser.dart:5: class RenamingUnparser extends Unparser { On 2012/07/23 11:23:10, Anton Mukhin wrote: > do we need both RenamingUnparser and Unparser? I don't think so. Merged. https://chromiumcodereview.appspot.com/10795064/diff/1/lib/compiler/implement... lib/compiler/implementation/dart_backend/renaming_unparser.dart:11: if (node.selector !== null && node.selector is TypeAnnotation) { On 2012/07/23 11:23:10, Anton Mukhin wrote: > no need in !== check, null is TypeAnnotation should be false, if I remember it > right. (null is AnyType) == true https://chromiumcodereview.appspot.com/10795064/diff/1/tests/compiler/dart2js... File tests/compiler/dart2js/unparser_test.dart (right): https://chromiumcodereview.appspot.com/10795064/diff/1/tests/compiler/dart2js... tests/compiler/dart2js/unparser_test.dart:70: fileUri(path) => new Uri(scheme: 'file', path: path); On 2012/07/23 11:23:10, Anton Mukhin wrote: > looks like a lot of code duplication w/ testDart2Dart, rework? Done. https://chromiumcodereview.appspot.com/10795064/diff/1/tests/compiler/dart2js... tests/compiler/dart2js/unparser_test.dart:251: mylib.A getA() { On 2012/07/23 11:23:10, Anton Mukhin wrote: > nit: => syntax, please Done.
I am still not quite happy with those checks for factories, etc., but I would leave it up to you if you want this to go in or not. https://chromiumcodereview.appspot.com/10795064/diff/1/lib/compiler/implement... File lib/compiler/implementation/dart_backend/renamer.dart (right): https://chromiumcodereview.appspot.com/10795064/diff/1/lib/compiler/implement... lib/compiler/implementation/dart_backend/renamer.dart:48: if (contextElements[send] !== null && contextElements[send].isFunction()) { Shouldn't you rename them as well? On 2012/07/23 15:34:15, Roman wrote: > On 2012/07/23 11:23:10, Anton Mukhin wrote: > > what about top-level variables? > > I don't quite understand. What about them? https://chromiumcodereview.appspot.com/10795064/diff/1/lib/compiler/implement... lib/compiler/implementation/dart_backend/renamer.dart:56: if (contextElements == null On 2012/07/23 15:34:15, Roman wrote: > On 2012/07/23 11:23:10, Anton Mukhin wrote: > > how contextElements can be null? > > this happens for class member fields. why? and, please, assert. https://chromiumcodereview.appspot.com/10795064/diff/1/lib/compiler/implement... lib/compiler/implementation/dart_backend/renamer.dart:68: if (typeAnnotation.typeName is Send On 2012/07/23 15:34:15, Roman wrote: > On 2012/07/23 11:23:10, Anton Mukhin wrote: > > what about generic types? > > Generics are in type arguments. Here we care only about the "core" type. Sorry, do you process generics correctly or not? https://chromiumcodereview.appspot.com/10795064/diff/1/lib/compiler/implement... lib/compiler/implementation/dart_backend/renamer.dart:94: return null; :) I think we should rename both top level vars and private names. On 2012/07/23 15:34:15, Roman wrote: > On 2012/07/23 11:23:10, Anton Mukhin wrote: > > again, what about top-level fields and privates? > > Again please elaborate :) https://chromiumcodereview.appspot.com/10795064/diff/10001/lib/compiler/imple... File lib/compiler/implementation/dart_backend/emitter.dart (right): https://chromiumcodereview.appspot.com/10795064/diff/10001/lib/compiler/imple... lib/compiler/implementation/dart_backend/emitter.dart:41: sb.add(interfaceNodes.head); shouldn't you rename those types? https://chromiumcodereview.appspot.com/10795064/diff/10001/lib/compiler/imple... File lib/compiler/implementation/dart_backend/renamer.dart (right): https://chromiumcodereview.appspot.com/10795064/diff/10001/lib/compiler/imple... lib/compiler/implementation/dart_backend/renamer.dart:44: if (contextElements === null please, comment what goes on here. https://chromiumcodereview.appspot.com/10795064/diff/10001/lib/compiler/imple... File lib/compiler/implementation/tree/renamer.dart (right): https://chromiumcodereview.appspot.com/10795064/diff/10001/lib/compiler/imple... lib/compiler/implementation/tree/renamer.dart:29: class NoRenamer implements Renamer { instead of additional class, null might be default implementation. https://chromiumcodereview.appspot.com/10795064/diff/10001/lib/compiler/imple... File lib/compiler/implementation/tree/unparser.dart (right): https://chromiumcodereview.appspot.com/10795064/diff/10001/lib/compiler/imple... lib/compiler/implementation/tree/unparser.dart:122: if (newName == null) { if instead of null, renamer would have returned always a name (maybe the same as passed in), the code below would be notably simpler. https://chromiumcodereview.appspot.com/10795064/diff/10001/lib/compiler/imple... lib/compiler/implementation/tree/unparser.dart:252: unparseSend(node); what is the case this covers? https://chromiumcodereview.appspot.com/10795064/diff/10001/lib/compiler/imple... lib/compiler/implementation/tree/unparser.dart:306: String newName = renamer.renameTypeName(node); ditto for null https://chromiumcodereview.appspot.com/10795064/diff/10001/tests/compiler/dar... File tests/compiler/dart2js/unparser_test.dart (right): https://chromiumcodereview.appspot.com/10795064/diff/10001/tests/compiler/dar... tests/compiler/dart2js/unparser_test.dart:38: if (continuation == null) { nit: === null https://chromiumcodereview.appspot.com/10795064/diff/10001/tests/compiler/dar... tests/compiler/dart2js/unparser_test.dart:39: continuation = (s) => Expect.equals(src, s); technically, it should be warning as Expect.equals returns nothing
https://chromiumcodereview.appspot.com/10795064/diff/1/lib/compiler/implement... File lib/compiler/implementation/dart_backend/renamer.dart (right): https://chromiumcodereview.appspot.com/10795064/diff/1/lib/compiler/implement... lib/compiler/implementation/dart_backend/renamer.dart:56: if (contextElements == null On 2012/07/23 17:01:27, Anton Mukhin wrote: > On 2012/07/23 15:34:15, Roman wrote: > > On 2012/07/23 11:23:10, Anton Mukhin wrote: > > > how contextElements can be null? > > > > this happens for class member fields. > > why? and, please, assert. > I'm not sure why. Maybe resolver needs some fixes. Added a TODO. https://chromiumcodereview.appspot.com/10795064/diff/1/lib/compiler/implement... lib/compiler/implementation/dart_backend/renamer.dart:68: if (typeAnnotation.typeName is Send On 2012/07/23 17:01:27, Anton Mukhin wrote: > On 2012/07/23 15:34:15, Roman wrote: > > On 2012/07/23 11:23:10, Anton Mukhin wrote: > > > what about generic types? > > > > Generics are in type arguments. Here we care only about the "core" type. > > Sorry, do you process generics correctly or not? Yes. https://chromiumcodereview.appspot.com/10795064/diff/10001/lib/compiler/imple... File lib/compiler/implementation/dart_backend/emitter.dart (right): https://chromiumcodereview.appspot.com/10795064/diff/10001/lib/compiler/imple... lib/compiler/implementation/dart_backend/emitter.dart:41: sb.add(interfaceNodes.head); On 2012/07/23 17:01:27, Anton Mukhin wrote: > shouldn't you rename those types? Done. https://chromiumcodereview.appspot.com/10795064/diff/10001/lib/compiler/imple... File lib/compiler/implementation/dart_backend/renamer.dart (right): https://chromiumcodereview.appspot.com/10795064/diff/10001/lib/compiler/imple... lib/compiler/implementation/dart_backend/renamer.dart:44: if (contextElements === null On 2012/07/23 17:01:27, Anton Mukhin wrote: > please, comment what goes on here. Done. https://chromiumcodereview.appspot.com/10795064/diff/10001/lib/compiler/imple... File lib/compiler/implementation/tree/renamer.dart (right): https://chromiumcodereview.appspot.com/10795064/diff/10001/lib/compiler/imple... lib/compiler/implementation/tree/renamer.dart:29: class NoRenamer implements Renamer { On 2012/07/23 17:01:27, Anton Mukhin wrote: > instead of additional class, null might be default implementation. Done. https://chromiumcodereview.appspot.com/10795064/diff/10001/lib/compiler/imple... File lib/compiler/implementation/tree/unparser.dart (right): https://chromiumcodereview.appspot.com/10795064/diff/10001/lib/compiler/imple... lib/compiler/implementation/tree/unparser.dart:122: if (newName == null) { On 2012/07/23 17:01:27, Anton Mukhin wrote: > if instead of null, renamer would have returned always a name (maybe the same as > passed in), the code below would be notably simpler. Yes, I thought about that, but the code in both NoRenamer and ConflictingRenamer will be more complicated, so I returned to this way. https://chromiumcodereview.appspot.com/10795064/diff/10001/lib/compiler/imple... lib/compiler/implementation/tree/unparser.dart:252: unparseSend(node); On 2012/07/23 17:01:27, Anton Mukhin wrote: > what is the case this covers? Thanks! I don't think it affects anything now. Removed. https://chromiumcodereview.appspot.com/10795064/diff/10001/tests/compiler/dar... File tests/compiler/dart2js/unparser_test.dart (right): https://chromiumcodereview.appspot.com/10795064/diff/10001/tests/compiler/dar... tests/compiler/dart2js/unparser_test.dart:38: if (continuation == null) { On 2012/07/23 17:01:27, Anton Mukhin wrote: > nit: === null Done. https://chromiumcodereview.appspot.com/10795064/diff/10001/tests/compiler/dar... tests/compiler/dart2js/unparser_test.dart:39: continuation = (s) => Expect.equals(src, s); On 2012/07/23 17:01:27, Anton Mukhin wrote: > technically, it should be warning as Expect.equals returns nothing Changed to (s) { Expect.equals(src, s); }
ok, let's start w/ it as a base approach, although I am very uneasy about it. lgtm https://chromiumcodereview.appspot.com/10795064/diff/7011/lib/compiler/implem... File lib/compiler/implementation/dart_backend/emitter.dart (right): https://chromiumcodereview.appspot.com/10795064/diff/7011/lib/compiler/implem... lib/compiler/implementation/dart_backend/emitter.dart:40: Link<Node> interfaceNodes = classNode.interfaces.nodes; your modified version of NodeList processing, won't it cover this case? I mean won't unparser.unparse(classNode.interfaces) just work? https://chromiumcodereview.appspot.com/10795064/diff/7011/lib/compiler/implem... File lib/compiler/implementation/dart_backend/renamer.dart (right): https://chromiumcodereview.appspot.com/10795064/diff/7011/lib/compiler/implem... lib/compiler/implementation/dart_backend/renamer.dart:29: * Renames method name. Returns [null] if no rename is needed. nit: I don't think you need to duplicate documentation of the methods. https://chromiumcodereview.appspot.com/10795064/diff/7011/lib/compiler/implem... lib/compiler/implementation/dart_backend/renamer.dart:29: * Renames method name. Returns [null] if no rename is needed. nit: I don't think you need to duplicate documentation. https://chromiumcodereview.appspot.com/10795064/diff/7011/lib/compiler/implem... lib/compiler/implementation/dart_backend/renamer.dart:96: if (element == null || element.getLibrary() == compiler.coreLibrary nit: === null, but more importantly: you just did element.name in previous line, so you're dead if it's null
https://chromiumcodereview.appspot.com/10795064/diff/7011/lib/compiler/implem... File lib/compiler/implementation/dart_backend/emitter.dart (right): https://chromiumcodereview.appspot.com/10795064/diff/7011/lib/compiler/implem... lib/compiler/implementation/dart_backend/emitter.dart:40: Link<Node> interfaceNodes = classNode.interfaces.nodes; On 2012/07/23 17:58:54, Anton Mukhin wrote: > your modified version of NodeList processing, won't it cover this case? I mean > won't unparser.unparse(classNode.interfaces) just work? Unfortunately, unparse() for NodeList first prints out begin token (which is 'extends' or 'implements') and then, without any whitespaces, a list of unparsed nodes. So it would be 'extendsA,B,C'. https://chromiumcodereview.appspot.com/10795064/diff/7011/lib/compiler/implem... File lib/compiler/implementation/dart_backend/renamer.dart (right): https://chromiumcodereview.appspot.com/10795064/diff/7011/lib/compiler/implem... lib/compiler/implementation/dart_backend/renamer.dart:29: * Renames method name. Returns [null] if no rename is needed. On 2012/07/23 17:58:54, Anton Mukhin wrote: > nit: I don't think you need to duplicate documentation of the methods. Done. https://chromiumcodereview.appspot.com/10795064/diff/7011/lib/compiler/implem... lib/compiler/implementation/dart_backend/renamer.dart:96: if (element == null || element.getLibrary() == compiler.coreLibrary On 2012/07/23 17:58:54, Anton Mukhin wrote: > nit: === null, but more importantly: you just did element.name in previous line, > so you're dead if it's null Correct, removed null check. https://chromiumcodereview.appspot.com/10795064/diff/7011/lib/compiler/implem... File lib/compiler/implementation/resolver.dart (right): https://chromiumcodereview.appspot.com/10795064/diff/7011/lib/compiler/implem... lib/compiler/implementation/resolver.dart:956: if (variableDefinitions.type !== null) { I removed this because it is the reason of stack overflow in one test.
still lgtm https://chromiumcodereview.appspot.com/10795064/diff/7011/lib/compiler/implem... File lib/compiler/implementation/dart_backend/emitter.dart (right): https://chromiumcodereview.appspot.com/10795064/diff/7011/lib/compiler/implem... lib/compiler/implementation/dart_backend/emitter.dart:40: Link<Node> interfaceNodes = classNode.interfaces.nodes; On 2012/07/24 08:22:49, Roman wrote: > On 2012/07/23 17:58:54, Anton Mukhin wrote: > > your modified version of NodeList processing, won't it cover this case? I > mean > > won't unparser.unparse(classNode.interfaces) just work? > > Unfortunately, unparse() for NodeList first prints out begin token (which is > 'extends' or 'implements') and then, without any whitespaces, a list of unparsed > nodes. So it would be 'extendsA,B,C'. Shouldn't that be fixed?
https://chromiumcodereview.appspot.com/10795064/diff/7011/lib/compiler/implem... File lib/compiler/implementation/dart_backend/emitter.dart (right): https://chromiumcodereview.appspot.com/10795064/diff/7011/lib/compiler/implem... lib/compiler/implementation/dart_backend/emitter.dart:40: Link<Node> interfaceNodes = classNode.interfaces.nodes; On 2012/07/24 08:43:58, Anton Mukhin wrote: > On 2012/07/24 08:22:49, Roman wrote: > > On 2012/07/23 17:58:54, Anton Mukhin wrote: > > > your modified version of NodeList processing, won't it cover this case? I > > mean > > > won't unparser.unparse(classNode.interfaces) just work? > > > > Unfortunately, unparse() for NodeList first prints out begin token (which is > > 'extends' or 'implements') and then, without any whitespaces, a list of > unparsed > > nodes. So it would be 'extendsA,B,C'. > > Shouldn't that be fixed? It is not clear that it's a bug. Usually node list is something like parameters for a function, e.g. foo(a,b,c), or a list of statements in a function body, you don't need whitespaces there.
https://chromiumcodereview.appspot.com/10795064/diff/7011/lib/compiler/implem... File lib/compiler/implementation/dart_backend/emitter.dart (right): https://chromiumcodereview.appspot.com/10795064/diff/7011/lib/compiler/implem... lib/compiler/implementation/dart_backend/emitter.dart:40: Link<Node> interfaceNodes = classNode.interfaces.nodes; On 2012/07/24 08:58:04, Roman wrote: > On 2012/07/24 08:43:58, Anton Mukhin wrote: > > On 2012/07/24 08:22:49, Roman wrote: > > > On 2012/07/23 17:58:54, Anton Mukhin wrote: > > > > your modified version of NodeList processing, won't it cover this case? I > > > mean > > > > won't unparser.unparse(classNode.interfaces) just work? > > > > > > Unfortunately, unparse() for NodeList first prints out begin token (which is > > > 'extends' or 'implements') and then, without any whitespaces, a list of > > unparsed > > > nodes. So it would be 'extendsA,B,C'. > > > > Shouldn't that be fixed? > > It is not clear that it's a bug. Usually node list is something like parameters > for a function, e.g. foo(a,b,c), or a list of statements in a function body, you > don't need whitespaces there. For a case of whitespaces there is no beginToken I guess, so that example is bad.
https://chromiumcodereview.appspot.com/10795064/diff/7011/lib/compiler/implem... File lib/compiler/implementation/dart_backend/emitter.dart (right): https://chromiumcodereview.appspot.com/10795064/diff/7011/lib/compiler/implem... lib/compiler/implementation/dart_backend/emitter.dart:40: Link<Node> interfaceNodes = classNode.interfaces.nodes; I think it should be fixed even if (for now) we'll output unnecessary space: I believe unparse should be able to treat any node list correctly at node list level. On 2012/07/24 08:58:54, Roman wrote: > On 2012/07/24 08:58:04, Roman wrote: > > On 2012/07/24 08:43:58, Anton Mukhin wrote: > > > On 2012/07/24 08:22:49, Roman wrote: > > > > On 2012/07/23 17:58:54, Anton Mukhin wrote: > > > > > your modified version of NodeList processing, won't it cover this case? > I > > > > mean > > > > > won't unparser.unparse(classNode.interfaces) just work? > > > > > > > > Unfortunately, unparse() for NodeList first prints out begin token (which > is > > > > 'extends' or 'implements') and then, without any whitespaces, a list of > > > unparsed > > > > nodes. So it would be 'extendsA,B,C'. > > > > > > Shouldn't that be fixed? > > > > It is not clear that it's a bug. Usually node list is something like > parameters > > for a function, e.g. foo(a,b,c), or a list of statements in a function body, > you > > don't need whitespaces there. > > For a case of whitespaces there is no beginToken I guess, so that example is > bad.
https://chromiumcodereview.appspot.com/10795064/diff/7011/lib/compiler/implem... File lib/compiler/implementation/dart_backend/emitter.dart (right): https://chromiumcodereview.appspot.com/10795064/diff/7011/lib/compiler/implem... lib/compiler/implementation/dart_backend/emitter.dart:40: Link<Node> interfaceNodes = classNode.interfaces.nodes; On 2012/07/24 09:00:10, Anton Mukhin wrote: > I think it should be fixed even if (for now) we'll output unnecessary space: I > believe unparse should be able to treat any node list correctly at node list > level. > > On 2012/07/24 08:58:54, Roman wrote: > > On 2012/07/24 08:58:04, Roman wrote: > > > On 2012/07/24 08:43:58, Anton Mukhin wrote: > > > > On 2012/07/24 08:22:49, Roman wrote: > > > > > On 2012/07/23 17:58:54, Anton Mukhin wrote: > > > > > > your modified version of NodeList processing, won't it cover this > case? > > I > > > > > mean > > > > > > won't unparser.unparse(classNode.interfaces) just work? > > > > > > > > > > Unfortunately, unparse() for NodeList first prints out begin token > (which > > is > > > > > 'extends' or 'implements') and then, without any whitespaces, a list of > > > > unparsed > > > > > nodes. So it would be 'extendsA,B,C'. > > > > > > > > Shouldn't that be fixed? > > > > > > It is not clear that it's a bug. Usually node list is something like > > parameters > > > for a function, e.g. foo(a,b,c), or a list of statements in a function body, > > you > > > don't need whitespaces there. > > > > For a case of whitespaces there is no beginToken I guess, so that example is > > bad. > I was wrong, 'extends' or 'implements' is not part of interfaces nodelist. So I just changed this part to unparser.unparse(classNode.interfaces);
lgtm https://chromiumcodereview.appspot.com/10795064/diff/7011/lib/compiler/implem... File lib/compiler/implementation/dart_backend/emitter.dart (right): https://chromiumcodereview.appspot.com/10795064/diff/7011/lib/compiler/implem... lib/compiler/implementation/dart_backend/emitter.dart:40: Link<Node> interfaceNodes = classNode.interfaces.nodes; thanks a lot! On 2012/07/24 10:37:04, Roman wrote: > On 2012/07/24 09:00:10, Anton Mukhin wrote: > > I think it should be fixed even if (for now) we'll output unnecessary space: I > > believe unparse should be able to treat any node list correctly at node list > > level. > > > > On 2012/07/24 08:58:54, Roman wrote: > > > On 2012/07/24 08:58:04, Roman wrote: > > > > On 2012/07/24 08:43:58, Anton Mukhin wrote: > > > > > On 2012/07/24 08:22:49, Roman wrote: > > > > > > On 2012/07/23 17:58:54, Anton Mukhin wrote: > > > > > > > your modified version of NodeList processing, won't it cover this > > case? > > > I > > > > > > mean > > > > > > > won't unparser.unparse(classNode.interfaces) just work? > > > > > > > > > > > > Unfortunately, unparse() for NodeList first prints out begin token > > (which > > > is > > > > > > 'extends' or 'implements') and then, without any whitespaces, a list > of > > > > > unparsed > > > > > > nodes. So it would be 'extendsA,B,C'. > > > > > > > > > > Shouldn't that be fixed? > > > > > > > > It is not clear that it's a bug. Usually node list is something like > > > parameters > > > > for a function, e.g. foo(a,b,c), or a list of statements in a function > body, > > > you > > > > don't need whitespaces there. > > > > > > For a case of whitespaces there is no beginToken I guess, so that example is > > > bad. > > > > I was wrong, 'extends' or 'implements' is not part of interfaces nodelist. So I > just changed this part to unparser.unparse(classNode.interfaces);
https://chromiumcodereview.appspot.com/10795064/diff/6013/lib/compiler/implem... File lib/compiler/implementation/resolver.dart (right): https://chromiumcodereview.appspot.com/10795064/diff/6013/lib/compiler/implem... lib/compiler/implementation/resolver.dart:944: if (node.returnType !== null) resolveTypeAnnotation(node.returnType); I think you should be able to revert this since you should be using the FunctionElement moving forward.
https://chromiumcodereview.appspot.com/10795064/diff/6013/lib/compiler/implem... File lib/compiler/implementation/tree/unparser.dart (right): https://chromiumcodereview.appspot.com/10795064/diff/6013/lib/compiler/implem... lib/compiler/implementation/tree/unparser.dart:6: final Renamer renamer; I'm not really comfortable with this addition. The unparser is used by dart2js for debugging and testing. Renaming is not something that the unparser should be dealing with. I think renaming can be handled by a subclass of Unparser.
https://chromiumcodereview.appspot.com/10795064/diff/6013/lib/compiler/implem... File lib/compiler/implementation/tree/unparser.dart (right): https://chromiumcodereview.appspot.com/10795064/diff/6013/lib/compiler/implem... lib/compiler/implementation/tree/unparser.dart:6: final Renamer renamer; I agree with you, Peter, see https://chromiumcodereview.appspot.com/10905019/ Let me discuss it with Roma once again On 2012/10/01 11:27:27, ahe wrote: > I'm not really comfortable with this addition. The unparser is used by dart2js > for debugging and testing. Renaming is not something that the unparser should be > dealing with. > > I think renaming can be handled by a subclass of Unparser.
https://chromiumcodereview.appspot.com/10795064/diff/6013/lib/compiler/implem... File lib/compiler/implementation/tree/unparser.dart (right): https://chromiumcodereview.appspot.com/10795064/diff/6013/lib/compiler/implem... lib/compiler/implementation/tree/unparser.dart:6: final Renamer renamer; Thank you, good that we agree on the long term direction. On 2012/10/01 13:10:08, Anton Muhin wrote: > I agree with you, Peter, see https://chromiumcodereview.appspot.com/10905019/ > > Let me discuss it with Roma once again > > On 2012/10/01 11:27:27, ahe wrote: > > I'm not really comfortable with this addition. The unparser is used by dart2js > > for debugging and testing. Renaming is not something that the unparser should > be > > dealing with. > > > > I think renaming can be handled by a subclass of Unparser. > |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
