Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10383311:
Support proper unparse of is and is!. (Closed)
|
Created:
8 years, 7 months ago by Anton Muhin Modified:
8 years, 6 months ago CC:
reviews_dartlang.org Visibility:
Public. |
DescriptionSupport proper unparse of is and is!.
R=ahe@google.com,smok@google.com
Committed: https://code.google.com/p/dart/source/detail?r=8193
Patch Set 1 #Patch Set 2 : Remove blank line #Patch Set 3 : #
Total comments: 9
Patch Set 4 : #
Total comments: 7
Patch Set 5 : Next iteration #Messages
Total messages: 10 (0 generated)
https://chromiumcodereview.appspot.com/10383311/diff/5001/lib/compiler/implem... File lib/compiler/implementation/tree/unparser.dart (right): https://chromiumcodereview.appspot.com/10383311/diff/5001/lib/compiler/implem... lib/compiler/implementation/tree/unparser.dart:197: // Special case. Simple expr is T is represented as Send(expr, 'is', T), this is rather ugly. I try to emit expr is! Type exactly like that (we can probably save one space here). Peter, why do we have such a representation of is! ?
https://chromiumcodereview.appspot.com/10383311/diff/5001/lib/compiler/implem... File lib/compiler/implementation/tree/unparser.dart (right): https://chromiumcodereview.appspot.com/10383311/diff/5001/lib/compiler/implem... lib/compiler/implementation/tree/unparser.dart:187: if (node.receiver !== null) { How about this: Operator op = node.selector.asOperator(); bool isCheck = op !== null && op.source.stringValue === 'is'; if (node.receiver !== null) { visit(node.receiver); if (op === null) { sb.add('.'); } else if (isCheck) { sb.add(' '); } } if (!node.isPrefix) { visit(node.selector); } if (isCheck) sb.add(' '); https://chromiumcodereview.appspot.com/10383311/diff/5001/lib/compiler/implem... lib/compiler/implementation/tree/unparser.dart:197: // Special case. Simple expr is T is represented as Send(expr, 'is', T), On 2012/05/23 19:41:11, antonmuhin wrote: > this is rather ugly. > > I try to emit expr is! Type exactly like that (we can probably save one space > here). > > Peter, why do we have such a representation of is! ? This is the result of an allergic reaction to http://code.google.com/p/dart/source/browse/branches/bleeding_edge/dart/compi... Perhaps we went too far in the opposite direction. However, there are reasons for the madness. The less classes the AST hierarchy is composed of, the more flexible it is. Flexibility is important for dealing with syntax errors. For example, we could have used this AST node to represent an is-check: class IsCheck extends Node { Expression expression; TypeAnnotation typeAnnotation; bool isNegative; } But then we wouldn't be able to represent a parse tree for this bogus code: if (foo is (1+2)) { ... } Representing junk like that is important for an editor to be able to do code completion at any point. The problem we haven't solved is how to make it convenient to work with the more flexible AST nodes. https://chromiumcodereview.appspot.com/10383311/diff/5001/lib/compiler/implem... lib/compiler/implementation/tree/unparser.dart:197: // Special case. Simple expr is T is represented as Send(expr, 'is', T), Extra space after period. https://chromiumcodereview.appspot.com/10383311/diff/5001/lib/compiler/implem... lib/compiler/implementation/tree/unparser.dart:197: // Special case. Simple expr is T is represented as Send(expr, 'is', T), I suggest that you put code snippets in [:...:], for example: // Special case. Simple [:expr is T:] is represented as [:Send(expr, 'is', T):],
https://chromiumcodereview.appspot.com/10383311/diff/5001/lib/compiler/implem... File lib/compiler/implementation/tree/unparser.dart (right): https://chromiumcodereview.appspot.com/10383311/diff/5001/lib/compiler/implem... lib/compiler/implementation/tree/unparser.dart:187: if (node.receiver !== null) { Done with following changes: 1) == 'is' instead of === 'is' (does language guarantees that === will wok here?) 2) I need slightly more involved logic: I don't emit trailing space for is! form and hence I get 'expr is!Type' which is apparently correct Dart; 3) I didn't use if (isCheck) sb.add(' ') to follow the style of the function. I can turn all ifs into single-lines if you prefer that. On 2012/05/24 09:30:52, ahe wrote: > How about this: > > Operator op = node.selector.asOperator(); > bool isCheck = op !== null && op.source.stringValue === 'is'; > if (node.receiver !== null) { > visit(node.receiver); > if (op === null) { > sb.add('.'); > } else if (isCheck) { > sb.add(' '); > } > } > if (!node.isPrefix) { > visit(node.selector); > } > if (isCheck) sb.add(' '); https://chromiumcodereview.appspot.com/10383311/diff/5001/lib/compiler/implem... lib/compiler/implementation/tree/unparser.dart:197: // Special case. Simple expr is T is represented as Send(expr, 'is', T), I see, still prefixing a type with negation looks like a hack to me, I'd rather have more something like; expr is Type = Send(expr, Operator('is'), Type) and expr is! Type = Send(expr, Operator('is!'), Type or something of that kind. On 2012/05/24 09:30:52, ahe wrote: > On 2012/05/23 19:41:11, antonmuhin wrote: > > this is rather ugly. > > > > I try to emit expr is! Type exactly like that (we can probably save one space > > here). > > > > Peter, why do we have such a representation of is! ? > > This is the result of an allergic reaction to > http://code.google.com/p/dart/source/browse/branches/bleeding_edge/dart/compi... > > Perhaps we went too far in the opposite direction. > > However, there are reasons for the madness. The less classes the AST hierarchy > is composed of, the more flexible it is. Flexibility is important for dealing > with syntax errors. > > For example, we could have used this AST node to represent an is-check: > > class IsCheck extends Node { > Expression expression; > TypeAnnotation typeAnnotation; > bool isNegative; > } > > But then we wouldn't be able to represent a parse tree for this bogus code: > > if (foo is (1+2)) { ... } > > Representing junk like that is important for an editor to be able to do code > completion at any point. > > The problem we haven't solved is how to make it convenient to work with the more > flexible AST nodes. https://chromiumcodereview.appspot.com/10383311/diff/5001/lib/compiler/implem... lib/compiler/implementation/tree/unparser.dart:197: // Special case. Simple expr is T is represented as Send(expr, 'is', T), Now obsolete. So you don't use two spaces after period, will mark. On 2012/05/24 09:30:52, ahe wrote: > Extra space after period. https://chromiumcodereview.appspot.com/10383311/diff/5001/lib/compiler/implem... lib/compiler/implementation/tree/unparser.dart:197: // Special case. Simple expr is T is represented as Send(expr, 'is', T), Now obsolete, but will do next time. On 2012/05/24 09:30:52, ahe wrote: > I suggest that you put code snippets in [:...:], for example: > > // Special case. Simple [:expr is T:] is represented as [:Send(expr, 'is', > T):],
friendly ping
https://chromiumcodereview.appspot.com/10383311/diff/9001/lib/compiler/implem... File lib/compiler/implementation/tree/unparser.dart (right): https://chromiumcodereview.appspot.com/10383311/diff/9001/lib/compiler/implem... lib/compiler/implementation/tree/unparser.dart:186: // is check requires trailing space if it's not is! form. Not a proper sentence. First word should be capitalized. https://chromiumcodereview.appspot.com/10383311/diff/9001/lib/compiler/implem... lib/compiler/implementation/tree/unparser.dart:187: bool requiresSpace = isCheck && node.arguments.head is! Send; The style we use is this: node.arguments.head is !Send The space is before the !. https://chromiumcodereview.appspot.com/10383311/diff/9001/lib/compiler/implem... lib/compiler/implementation/tree/unparser.dart:203: if (requiresSpace) { This expression: 1 is !Object Is printed as: 1 is!Object If you change this into "if (isCheck) {", the expression is printed correctly. This makes the code simpler because you can remove two lines (186 and 187).
Random style comment. https://chromiumcodereview.appspot.com/10383311/diff/9001/lib/compiler/implem... File lib/compiler/implementation/tree/unparser.dart (right): https://chromiumcodereview.appspot.com/10383311/diff/9001/lib/compiler/implem... lib/compiler/implementation/tree/unparser.dart:187: bool requiresSpace = isCheck && node.arguments.head is! Send; On 2012/05/29 13:40:07, ahe wrote: > The style we use is this: > > node.arguments.head is !Send > > The space is before the !. This was a surprise to me. In SEA I think we all use "foo is! Foo". But indeed there are twice as many uses of "is !" than "is! " in the repo. I will clarify this in the style guide so we are consistently using "is !".
PTAL https://chromiumcodereview.appspot.com/10383311/diff/9001/lib/compiler/implem... File lib/compiler/implementation/tree/unparser.dart (right): https://chromiumcodereview.appspot.com/10383311/diff/9001/lib/compiler/implem... lib/compiler/implementation/tree/unparser.dart:186: // is check requires trailing space if it's not is! form. On 2012/05/29 13:40:07, ahe wrote: > Not a proper sentence. First word should be capitalized. Now obsolete. https://chromiumcodereview.appspot.com/10383311/diff/9001/lib/compiler/implem... lib/compiler/implementation/tree/unparser.dart:187: bool requiresSpace = isCheck && node.arguments.head is! Send; That was a surprise for me as well. On 2012/05/30 21:46:51, Bob Nystrom wrote: > On 2012/05/29 13:40:07, ahe wrote: > > The style we use is this: > > > > node.arguments.head is !Send > > > > The space is before the !. > > This was a surprise to me. In SEA I think we all use "foo is! Foo". But indeed > there are twice as many uses of "is !" than "is! " in the repo. I will clarify > this in the style guide so we are consistently using "is !". https://chromiumcodereview.appspot.com/10383311/diff/9001/lib/compiler/implem... lib/compiler/implementation/tree/unparser.dart:203: if (requiresSpace) { Yes, that was planned as I thought that preferred form is 'is!' and in this case we can save on a space here. On 2012/05/29 13:40:07, ahe wrote: > This expression: > > 1 is !Object > > Is printed as: > > 1 is!Object > > If you change this into "if (isCheck) {", the expression is printed correctly. > This makes the code simpler because you can remove two lines (186 and 187).
LGTM! |
