dart2dart: Logical operators and related rewrite rules in dart_tree.

dart2dart: Logical operators and related rewrite rules in dart_tree.

BUG=
R=kmillikin@google.com

Committed: https://code.google.com/p/dart/source/detail?r=36356

[asgerf, 2014-05-15 12:42:11]

Please nominate more reviewers if the pattern matching code is too dense.

[Kevin Millikin (Google), 2014-05-19 11:36:41]

I think there's a bug in some of the rewrites, but I might be missing something.

https://codereview.chromium.org/284213002/diff/60001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/dart_backend/dart_tree.dart
(right):

https://codereview.chromium.org/284213002/diff/60001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/dart_backend/dart_tree.dart:622: *
Performs the following three transformations on the tree:
following three transformations ==> following transformations

https://codereview.chromium.org/284213002/diff/60001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/dart_backend/dart_tree.dart:716:
Map<Label,Label> labelRedirects = <Label,Label>{};
Space after comma: Map<Label, Label>

https://codereview.chromium.org/284213002/diff/60001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/dart_backend/dart_tree.dart:871: Label
newTarget = labelRedirects[node.label] = redirect(next.target);
More readable:

Label newTarget = redirect(next.target);
labelRedirects[node.label] = newTarget;

https://codereview.chromium.org/284213002/diff/60001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/dart_backend/dart_tree.dart:875:
labelRedirects.remove(node.label); // Save some space.
I wouldn't bother doing this here (or at all).

https://codereview.chromium.org/284213002/diff/60001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/dart_backend/dart_tree.dart:1040: bool
tryCollapseIf(If outerIf, bool branch1, bool branch2) {
I don't think the caller should have to enumerate the possibilities for the
pattern.  It seems like a detail of the specific pattern leaking out to the call
site (is it ever useful to leave off a variation?).

I'd rather have this function handle the possibilities.  I'm not sure what's
best:

bool tryCollapseIf(If outerIf) {
  for (bool outerPolarity in [true, false]) {
    ...
  }
}

bool tryCollapseIf(If outerIf) {
  bool outerPolarity = true;
  do {
    ...
    outerPolarity = !outerPolarity;
  } while (!outerPolarity);  // Yuck?
}

or something else.

https://codereview.chromium.org/284213002/diff/60001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/dart_backend/dart_tree.dart:1086: ///
We apply the following two rule to [If] statements (see [visitIf]).
two rule ==> rules

https://codereview.chromium.org/284213002/diff/60001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/dart_backend/dart_tree.dart:1098: ///
This is to enable other rewrites, such branch swapping in an if. In some
such branch ==> such as branch

https://codereview.chromium.org/284213002/diff/60001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/dart_backend/dart_tree.dart:1221: for
(int i=0; i<node.arguments.length; i++) {
Spaces around = and <.  Also below.

Maybe we should introduce a rewriting visitor class that does the left-to-right
rewriting visit of Tree by default.  What do you think?

https://codereview.chromium.org/284213002/diff/60001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/dart_backend/dart_tree.dart:1312:
Expression visitLogicalOperator(LogicalOperator node) {
Blank line between methods.

https://codereview.chromium.org/284213002/diff/60001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/dart_backend/dart_tree.dart:1344: ///
Creates an equivalent boolean expression. The expression must occur in a
equivalent ==> equivalent simplified, perhaps?

Reasoning about the code below seems a bit easier if we state that the returned
expression is never a double negation Not(Not(...)).  Do you want to 'promise'
that?

https://codereview.chromium.org/284213002/diff/60001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/dart_backend/dart_tree.dart:1379: // x
? true : y  ==> x || y
I could well be missing something here, but I think that if !polarity, this
needs to be &&.  That is, !x && !y.

https://codereview.chromium.org/284213002/diff/60001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/dart_backend/dart_tree.dart:1385: // x
? false : y  ==> !x && y
Likewise, if !polarity this should be ||.  That is, x || !y.  Similarly for the
two other cases below.

https://codereview.chromium.org/284213002/diff/60001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/dart_backend/tree_tracer.dart
(right):

https://codereview.chromium.org/284213002/diff/60001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/dart_backend/tree_tracer.dart:280:
return node is Conditional || node is LogicalOperator;
The implementation doesn't seem to match the name.  I  worry that it either
doesn't scale or is a source of later bugs.

I would expect an implementation like !usesPrefixNotation(node) &&
!usesPostfixNotation(node).  Alternately, a different approach in terms of
precedence.

I guess it's fine for now.

https://codereview.chromium.org/284213002/diff/60001/tests/language/if_flatte...
File tests/language/if_flatten1_test.dart (right):

https://codereview.chromium.org/284213002/diff/60001/tests/language/if_flatte...
tests/language/if_flatten1_test.dart:10: global = v;
Are these tabs or a lot of spaces? :)

[asgerf, 2014-05-19 13:29:07]

https://codereview.chromium.org/284213002/diff/60001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/dart_backend/dart_tree.dart
(right):

https://codereview.chromium.org/284213002/diff/60001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/dart_backend/dart_tree.dart:622: *
Performs the following three transformations on the tree:
On 2014/05/19 11:36:41, kmillikin wrote:
> following three transformations ==> following transformations

Done.

https://codereview.chromium.org/284213002/diff/60001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/dart_backend/dart_tree.dart:716:
Map<Label,Label> labelRedirects = <Label,Label>{};
On 2014/05/19 11:36:41, kmillikin wrote:
> Space after comma: Map<Label, Label>

Done.

https://codereview.chromium.org/284213002/diff/60001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/dart_backend/dart_tree.dart:871: Label
newTarget = labelRedirects[node.label] = redirect(next.target);
On 2014/05/19 11:36:41, kmillikin wrote:
> More readable:
> 
> Label newTarget = redirect(next.target);
> labelRedirects[node.label] = newTarget;

Done.

https://codereview.chromium.org/284213002/diff/60001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/dart_backend/dart_tree.dart:1221: for
(int i=0; i<node.arguments.length; i++) {
On 2014/05/19 11:36:41, kmillikin wrote:
> Spaces around = and <.  Also below.
> 
> Maybe we should introduce a rewriting visitor class that does the
left-to-right
> rewriting visit of Tree by default.  What do you think?

Good idea. I'll look at that in another CL.

https://codereview.chromium.org/284213002/diff/60001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/dart_backend/tree_tracer.dart
(right):

https://codereview.chromium.org/284213002/diff/60001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/dart_backend/tree_tracer.dart:280:
return node is Conditional || node is LogicalOperator;
On 2014/05/19 11:36:41, kmillikin wrote:
> The implementation doesn't seem to match the name.  I  worry that it either
> doesn't scale or is a source of later bugs.
> 
> I would expect an implementation like !usesPrefixNotation(node) &&
> !usesPostfixNotation(node).  Alternately, a different approach in terms of
> precedence.
> 
> I guess it's fine for now.

Yeah we need proper precedence-awareness. Right now, things like (x ? y :
z).foo() will be printed as x ? y : z.foo() in the tracer.

https://codereview.chromium.org/284213002/diff/60001/tests/language/if_flatte...
File tests/language/if_flatten1_test.dart (right):

https://codereview.chromium.org/284213002/diff/60001/tests/language/if_flatte...
tests/language/if_flatten1_test.dart:10: global = v;
On 2014/05/19 11:36:41, kmillikin wrote:
> Are these tabs or a lot of spaces? :)

Tabs, but I am replacing them with spaces now.

[Kevin Millikin (Google), 2014-05-20 11:09:00]

LGTM.

https://codereview.chromium.org/284213002/diff/100001/tests/language/if_flatt...
File tests/language/if_flatten1_test.dart (right):

https://codereview.chromium.org/284213002/diff/100001/tests/language/if_flatt...
tests/language/if_flatten1_test.dart:10: global = v;
Now they look like four spaces, and they should be 2.

[asgerf, 2014-05-20 13:13:40]

Committed patchset #8 manually as r36356 (presubmit successful).