Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10828390:
dart2dart Fix factory for interfaces renames (Closed)
|
Created:
8 years, 4 months ago by Roman Modified:
8 years, 4 months ago Reviewers:
Anton Muhin CC:
reviews_dartlang.org Visibility:
Public. |
Descriptiondart2dart Fix factory for interfaces renames
Committed: https://code.google.com/p/dart/source/detail?r=11033
Patch Set 1 #
Total comments: 16
Patch Set 2 : #
Total comments: 4
Patch Set 3 : #Patch Set 4 : #
Total comments: 4
Patch Set 5 : #
Messages
Total messages: 9 (0 generated)
https://chromiumcodereview.appspot.com/10828390/diff/1/lib/compiler/implement... File lib/compiler/implementation/dart_backend/placeholder_collector.dart (right): https://chromiumcodereview.appspot.com/10828390/diff/1/lib/compiler/implement... lib/compiler/implementation/dart_backend/placeholder_collector.dart:146: if (factoryName.token.slowToString() SourceString's are comparable https://chromiumcodereview.appspot.com/10828390/diff/1/lib/compiler/implement... lib/compiler/implementation/dart_backend/placeholder_collector.dart:146: if (factoryName.token.slowToString() why do you need a check here? maybe just drop if check above (ln. 126)? https://chromiumcodereview.appspot.com/10828390/diff/1/lib/compiler/implement... lib/compiler/implementation/dart_backend/placeholder_collector.dart:153: makeElementPlaceholder((factoryName as Send).receiver, enclosingClass); nit: factoryName.asSend() https://chromiumcodereview.appspot.com/10828390/diff/1/lib/compiler/implement... lib/compiler/implementation/dart_backend/placeholder_collector.dart:153: makeElementPlaceholder((factoryName as Send).receiver, enclosingClass); nit: line too long
https://chromiumcodereview.appspot.com/10828390/diff/1/lib/compiler/implement... File lib/compiler/implementation/dart_backend/placeholder_collector.dart (right): https://chromiumcodereview.appspot.com/10828390/diff/1/lib/compiler/implement... lib/compiler/implementation/dart_backend/placeholder_collector.dart:146: if (factoryName.token.slowToString() On 2012/08/20 18:43:34, Anton Muhin wrote: > why do you need a check here? maybe just drop if check above (ln. 126)? The comment above is actually incorrect, that if is for renaming plain constructor: class C{ C() {} } https://chromiumcodereview.appspot.com/10828390/diff/1/lib/compiler/implement... lib/compiler/implementation/dart_backend/placeholder_collector.dart:146: if (factoryName.token.slowToString() On 2012/08/20 18:43:34, Anton Muhin wrote: > SourceString's are comparable Done. https://chromiumcodereview.appspot.com/10828390/diff/1/lib/compiler/implement... lib/compiler/implementation/dart_backend/placeholder_collector.dart:153: makeElementPlaceholder((factoryName as Send).receiver, enclosingClass); On 2012/08/20 18:43:34, Anton Muhin wrote: > nit: factoryName.asSend() Done. https://chromiumcodereview.appspot.com/10828390/diff/1/lib/compiler/implement... lib/compiler/implementation/dart_backend/placeholder_collector.dart:153: makeElementPlaceholder((factoryName as Send).receiver, enclosingClass); On 2012/08/20 18:43:34, Anton Muhin wrote: > nit: line too long Done.
https://chromiumcodereview.appspot.com/10828390/diff/1/lib/compiler/implement... File lib/compiler/implementation/dart_backend/placeholder_collector.dart (right): https://chromiumcodereview.appspot.com/10828390/diff/1/lib/compiler/implement... lib/compiler/implementation/dart_backend/placeholder_collector.dart:134: // interface I { I();} interface I default C ? https://chromiumcodereview.appspot.com/10828390/diff/1/lib/compiler/implement... lib/compiler/implementation/dart_backend/placeholder_collector.dart:137: && element.defaultImplementation !== element) { what this check does? https://chromiumcodereview.appspot.com/10828390/diff/1/lib/compiler/implement... lib/compiler/implementation/dart_backend/placeholder_collector.dart:144: // interface I { I(); } again, I default C? https://chromiumcodereview.appspot.com/10828390/diff/1/lib/compiler/implement... lib/compiler/implementation/dart_backend/placeholder_collector.dart:147: != implementingFactory.getEnclosingClass().name.slowToString()) { it might be more straightforward to compare for equality: factoryName.token == element.name. https://chromiumcodereview.appspot.com/10828390/diff/5001/lib/compiler/implem... File lib/compiler/implementation/dart_backend/placeholder_collector.dart (right): https://chromiumcodereview.appspot.com/10828390/diff/5001/lib/compiler/implem... lib/compiler/implementation/dart_backend/placeholder_collector.dart:126: if (nameNode.asIdentifier().token.slowToString() as you're touching this code anyway, maybe make it compare SourceStrings to unify with the code below? https://chromiumcodereview.appspot.com/10828390/diff/5001/lib/compiler/implem... lib/compiler/implementation/dart_backend/placeholder_collector.dart:153: makeElementPlaceholder(factoryName.asSend().receiver, enclosingClass); it looks like you don't check here if you should remove this part (cf. with if above and the way the code is structured in ln. 126--128. I wonder if you should factor out this common logic.
https://chromiumcodereview.appspot.com/10828390/diff/1/lib/compiler/implement... File lib/compiler/implementation/dart_backend/placeholder_collector.dart (right): https://chromiumcodereview.appspot.com/10828390/diff/1/lib/compiler/implement... lib/compiler/implementation/dart_backend/placeholder_collector.dart:134: // interface I { I();} On 2012/08/21 09:06:35, Anton Muhin wrote: > interface I default C ? Done. https://chromiumcodereview.appspot.com/10828390/diff/1/lib/compiler/implement... lib/compiler/implementation/dart_backend/placeholder_collector.dart:137: && element.defaultImplementation !== element) { On 2012/08/21 09:06:35, Anton Muhin wrote: > what this check does? Here we are processing interface constructor: interface I default C { I(); } defaultImplementation may point to constructor inside default class, to I() inside C: class C { factory I() {} } What we want to do here is to put placeholder for I() inside class C. https://chromiumcodereview.appspot.com/10828390/diff/1/lib/compiler/implement... lib/compiler/implementation/dart_backend/placeholder_collector.dart:144: // interface I { I(); } On 2012/08/21 09:06:35, Anton Muhin wrote: > again, I default C? Actually, here not necessary default. https://chromiumcodereview.appspot.com/10828390/diff/1/lib/compiler/implement... lib/compiler/implementation/dart_backend/placeholder_collector.dart:147: != implementingFactory.getEnclosingClass().name.slowToString()) { On 2012/08/21 09:06:35, Anton Muhin wrote: > it might be more straightforward to compare for equality: factoryName.token == > element.name. I changed that to factoryName.token == implementingFactory.getEnclosingClass().name in the previous patch set? https://chromiumcodereview.appspot.com/10828390/diff/5001/lib/compiler/implem... File lib/compiler/implementation/dart_backend/placeholder_collector.dart (right): https://chromiumcodereview.appspot.com/10828390/diff/5001/lib/compiler/implem... lib/compiler/implementation/dart_backend/placeholder_collector.dart:126: if (nameNode.asIdentifier().token.slowToString() On 2012/08/21 09:06:36, Anton Muhin wrote: > as you're touching this code anyway, maybe make it compare SourceStrings to > unify with the code below? Done. https://chromiumcodereview.appspot.com/10828390/diff/5001/lib/compiler/implem... lib/compiler/implementation/dart_backend/placeholder_collector.dart:153: makeElementPlaceholder(factoryName.asSend().receiver, enclosingClass); On 2012/08/21 09:06:36, Anton Muhin wrote: > it looks like you don't check here if you should remove this part (cf. with if > above and the way the code is structured in ln. 126--128. > > I wonder if you should factor out this common logic. I don't understand your comment., what should I remove?
PTAL
lgtm https://chromiumcodereview.appspot.com/10828390/diff/10001/lib/compiler/imple... File lib/compiler/implementation/dart_backend/placeholder_collector.dart (right): https://chromiumcodereview.appspot.com/10828390/diff/10001/lib/compiler/imple... lib/compiler/implementation/dart_backend/placeholder_collector.dart:108: void renameConstructorName(Node nameNode, ClassElement element) { rename/name sounds a big ugly, maybe something like tryMakeConstructorPlaceholder ? https://chromiumcodereview.appspot.com/10828390/diff/10001/lib/compiler/imple... lib/compiler/implementation/dart_backend/placeholder_collector.dart:147: renameConstructorName(implementingFactory.cachedNode.name, maybe move .name into renameConsturctorName--it's natural for it to take FunctionExpression
https://chromiumcodereview.appspot.com/10828390/diff/10001/lib/compiler/imple... File lib/compiler/implementation/dart_backend/placeholder_collector.dart (right): https://chromiumcodereview.appspot.com/10828390/diff/10001/lib/compiler/imple... lib/compiler/implementation/dart_backend/placeholder_collector.dart:108: void renameConstructorName(Node nameNode, ClassElement element) { On 2012/08/21 11:18:42, Anton Muhin wrote: > rename/name sounds a big ugly, maybe something like > tryMakeConstructorPlaceholder ? Renamed to tryMakeConstructorNamePlaceholder https://chromiumcodereview.appspot.com/10828390/diff/10001/lib/compiler/imple... lib/compiler/implementation/dart_backend/placeholder_collector.dart:147: renameConstructorName(implementingFactory.cachedNode.name, On 2012/08/21 11:18:42, Anton Muhin wrote: > maybe move .name into renameConsturctorName--it's natural for it to take > FunctionExpression Done.
lgtm |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
