dart2js cps: Introduce some built-in operators in type propagation.

dart2js cps: Introduce some built-in operators in type propagation.

There are also a number of somewhat unrelated changes to get
better type information out of the global type inference.
The dependency on TypedSelector is causing a lot of problems there.

BUG=
R=kmillikin@google.com

Committed: https://github.com/dart-lang/sdk/commit/eec1d88d9cdf77d4c2dd64a4b0e8965dd1040ba3

[asgerf, 2015-06-10 18:26:23]

asgerf@google.com changed reviewers:
+ kmillikin@google.com, sra@google.com

[asgerf, 2015-06-10 18:26:24]

https://codereview.chromium.org/1175973005/diff/1/pkg/compiler/lib/src/tree_i...
File pkg/compiler/lib/src/tree_ir/tree_ir_nodes.dart (right):

https://codereview.chromium.org/1175973005/diff/1/pkg/compiler/lib/src/tree_i...
pkg/compiler/lib/src/tree_ir/tree_ir_nodes.dart:405: //               Right now
the tree builder compiles IsFalsy to Not.
Pattern matching just becomes a bit harder if it's not its own node, and the CL
has already grown larger than I wanted it to.

[sra1, 2015-06-10 21:07:38]

DBC

https://chromiumcodereview.appspot.com/1175973005/diff/1/pkg/compiler/lib/src...
File pkg/compiler/lib/src/cps_ir/builtin_operator.dart (right):

https://chromiumcodereview.appspot.com/1175973005/diff/1/pkg/compiler/lib/src...
pkg/compiler/lib/src/cps_ir/builtin_operator.dart:6: // Both cps_ir_nodes and
tree_ir_nodes import and reexport this file.
re-export

https://chromiumcodereview.appspot.com/1175973005/diff/1/pkg/compiler/lib/src...
pkg/compiler/lib/src/cps_ir/builtin_operator.dart:16: /// argument; their
behaviour is unspecified if those requirements are violated.
Many of these operators implement Dart operators.
Where this is true, say so in the comment.

Many of these operators correspond directly to JavaScript operators.
Where this is true, say so in the comment.

For example, in JavaScript we can use === to implement Dart a==b and
identical(a,b) when both a and b are strings or when one is a string and the
other is a string or (Dart) null (implemented as either JS null or undefined).

https://chromiumcodereview.appspot.com/1175973005/diff/1/pkg/compiler/lib/src...
pkg/compiler/lib/src/cps_ir/builtin_operator.dart:18: NumPlus, NumMinus,
NumMultiply, NumAnd, NumOr, NumXor,
"NumPlus" and "NumMultiply" are inconsistent.
One might read "2+2" as "two plus two" but the operation is addition, giving the
verb "to add".
Add/Subtract/Multiply, not Plus/Minus/Times

https://chromiumcodereview.appspot.com/1175973005/diff/1/pkg/compiler/lib/src...
pkg/compiler/lib/src/cps_ir/builtin_operator.dart:19: NumLt, NumLe, NumGt,
NumGe,
Generally format the enum elements all on one line or one per line.

https://chromiumcodereview.appspot.com/1175973005/diff/1/pkg/compiler/lib/src...
pkg/compiler/lib/src/cps_ir/builtin_operator.dart:22: /// caveats regarding NaN
and +/-0).
Either explain the caveats or reference where they are explained.

https://chromiumcodereview.appspot.com/1175973005/diff/1/pkg/compiler/lib/src...
pkg/compiler/lib/src/cps_ir/builtin_operator.dart:34: /// - At least one
argument is null.
is null, or may be null?

https://chromiumcodereview.appspot.com/1175973005/diff/1/pkg/compiler/lib/src...
pkg/compiler/lib/src/cps_ir/builtin_operator.dart:35: /// - Arguments are both
strings, or both booleans, or both numbers.
If this holds, you would use StrictEq, no?

Perhaps you mean arguments are both (string or null), or both (boolean or null),
...

https://chromiumcodereview.appspot.com/1175973005/diff/1/pkg/compiler/lib/src...
pkg/compiler/lib/src/cps_ir/builtin_operator.dart:39: LooseNeq,
We prefer words.
'eq' is not a word, so I would use 'equals'.

LooseNeq -> LooseNotEquals etc

NumGe -> NumGreaterOrEquals

https://chromiumcodereview.appspot.com/1175973005/diff/1/pkg/compiler/lib/src...
pkg/compiler/lib/src/cps_ir/builtin_operator.dart:42: /// or null.
'Falsy' is a JavaScript concept, so also 'undefined'.

https://chromiumcodereview.appspot.com/1175973005/diff/1/pkg/compiler/lib/src...
File pkg/compiler/lib/src/cps_ir/cps_ir_tracer.dart (right):

https://chromiumcodereview.appspot.com/1175973005/diff/1/pkg/compiler/lib/src...
pkg/compiler/lib/src/cps_ir/cps_ir_tracer.dart:378: return 'ApplyBuiltinOperator
$operator ($args)';
Small nits:
1. You should almost always use interpolation instead of toString().
2. It is hardly worth having an extra local when you can write

  return 'ApplyBuiltinOperator ${node.operator} ($args)';

https://chromiumcodereview.appspot.com/1175973005/diff/1/pkg/compiler/lib/src...
File pkg/compiler/lib/src/cps_ir/type_propagation.dart (right):

https://chromiumcodereview.appspot.com/1175973005/diff/1/pkg/compiler/lib/src...
pkg/compiler/lib/src/cps_ir/type_propagation.dart:378: final Map<String,
BuiltinOperator> NumBinaryBuiltins = const {
Use

... = const <String, BuiltinIperator>{

to get checking on the indexer argument in checked mode.

https://chromiumcodereview.appspot.com/1175973005/diff/1/pkg/compiler/lib/src...
File pkg/compiler/lib/src/js_backend/codegen/codegen.dart (right):

https://chromiumcodereview.appspot.com/1175973005/diff/1/pkg/compiler/lib/src...
pkg/compiler/lib/src/js_backend/codegen/codegen.dart:710: return js.js('(# & #)
>>> 0', <js.Expression>[args[0], args[1]]);
We don't always need the >>> 0.
SSA has the type at this point so it omits the >>> 0 if the result is
non-negative or none of the targets care [e.g. ((a & b) & c)]

[asgerf, 2015-06-11 11:16:40]

https://codereview.chromium.org/1175973005/diff/1/pkg/compiler/lib/src/cps_ir...
File pkg/compiler/lib/src/cps_ir/builtin_operator.dart (right):

https://codereview.chromium.org/1175973005/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/builtin_operator.dart:6: // Both cps_ir_nodes and
tree_ir_nodes import and reexport this file.
On 2015/06/10 21:07:37, sra1 wrote:
> re-export

Done.

https://codereview.chromium.org/1175973005/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/builtin_operator.dart:16: /// argument; their
behaviour is unspecified if those requirements are violated.
On 2015/06/10 21:07:37, sra1 wrote:
> Many of these operators implement Dart operators.
> Where this is true, say so in the comment.
> 
> Many of these operators correspond directly to JavaScript operators.
> Where this is true, say so in the comment.
> 
> For example, in JavaScript we can use === to implement Dart a==b and
> identical(a,b) when both a and b are strings or when one is a string and the
> other is a string or (Dart) null (implemented as either JS null or undefined).
> 

If you are referring to the bitwise operators using Dart semantics instead of
JS, then yes, I agree. I'll clarify.

I'm a bit wary of using the correspondence to JS operators as a way of
specifying behavior, but I guess it's ok for the numeric operators.

https://codereview.chromium.org/1175973005/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/builtin_operator.dart:18: NumPlus, NumMinus,
NumMultiply, NumAnd, NumOr, NumXor,
On 2015/06/10 21:07:37, sra1 wrote:
> "NumPlus" and "NumMultiply" are inconsistent.
> One might read "2+2" as "two plus two" but the operation is addition, giving
the
> verb "to add".
> Add/Subtract/Multiply, not Plus/Minus/Times

Done.

https://codereview.chromium.org/1175973005/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/builtin_operator.dart:19: NumLt, NumLe, NumGt,
NumGe,
On 2015/06/10 21:07:37, sra1 wrote:
> Generally format the enum elements all on one line or one per line.

Done.

https://codereview.chromium.org/1175973005/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/builtin_operator.dart:22: /// caveats regarding NaN
and +/-0).
On 2015/06/10 21:07:37, sra1 wrote:
> Either explain the caveats or reference where they are explained.

Done.

https://codereview.chromium.org/1175973005/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/builtin_operator.dart:34: /// - At least one
argument is null.
On 2015/06/10 21:07:37, sra1 wrote:
> is null, or may be null?

Is null.

https://codereview.chromium.org/1175973005/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/builtin_operator.dart:35: /// - Arguments are both
strings, or both booleans, or both numbers.
On 2015/06/10 21:07:37, sra1 wrote:
> If this holds, you would use StrictEq, no?
> 
> Perhaps you mean arguments are both (string or null), or both (boolean or
null),
> ...

The two bullet points are disjunctive; only one needs to be satisfied.

If we know the second criterion holds, then StrictEq would work, but we might
not know which criterion is satisfied at runtime (it could alternate with each
evaluation).

I initially wrote a comment similar to yours above, but changed it because that
spec does not allow us to compare arbitrary Dart objects against null.

I know seems easier to just say "this is JS ==" but that just pushes the problem
into optimizations, whose code would then get flooded with comments explaining
why it is safe to do so and so.

https://codereview.chromium.org/1175973005/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/builtin_operator.dart:39: LooseNeq,
On 2015/06/10 21:07:37, sra1 wrote:
> We prefer words.
> 'eq' is not a word, so I would use 'equals'.
> 
> LooseNeq -> LooseNotEquals etc
> 
> NumGe -> NumGreaterOrEquals

Readability will degrade tremendously if we do this, because lines will have to
be split everywhere.

https://codereview.chromium.org/1175973005/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/builtin_operator.dart:42: /// or null.
On 2015/06/10 21:07:37, sra1 wrote:
> 'Falsy' is a JavaScript concept, so also 'undefined'.

It don't think it matters where the word "falsy" comes from.

I was on the fence about whether to mention 'undefined' here, but ultimately
decided against it.

The operators are specified in terms of the values manipulated by the IR, and
the IR has no notion of 'undefined' as a separate value. If we start to say
'undefined' here, it would be inconsistent with all the other places where we
just say 'null'.

I've added a comment on the enum itself clarifying that "null" always means Dart
null.

If we ever beef up the type inference to distinguish null from undefined, and
introduce operators that specialize based on this knowledge, then we might have
to qualify each mention of null as 'Dart null' or 'JS null', but right now there
is no need.

https://codereview.chromium.org/1175973005/diff/1/pkg/compiler/lib/src/cps_ir...
File pkg/compiler/lib/src/cps_ir/type_propagation.dart (right):

https://codereview.chromium.org/1175973005/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/type_propagation.dart:378: final Map<String,
BuiltinOperator> NumBinaryBuiltins = const {
On 2015/06/10 21:07:37, sra1 wrote:
> Use
> 
> ... = const <String, BuiltinIperator>{
> 
> to get checking on the indexer argument in checked mode.
> 

Done.

https://codereview.chromium.org/1175973005/diff/1/pkg/compiler/lib/src/js_bac...
File pkg/compiler/lib/src/js_backend/codegen/codegen.dart (right):

https://codereview.chromium.org/1175973005/diff/1/pkg/compiler/lib/src/js_bac...
pkg/compiler/lib/src/js_backend/codegen/codegen.dart:710: return js.js('(# & #)
>>> 0', <js.Expression>[args[0], args[1]]);
On 2015/06/10 21:07:37, sra1 wrote:
> We don't always need the >>> 0.
> SSA has the type at this point so it omits the >>> 0 if the result is
> non-negative or none of the targets care [e.g. ((a & b) & c)]

We should definitely have the same in the CPS pipeline, but there are a few ways
to do this, and I did not want to commit to one of them yet.

We could have an explicit UInt coercion operator or a separate coercing and
non-coercing version of each bitwise operator.

Alternatively we could attach a bitmask with some type information on each
expression. It seems tempting, but I think it's a slippery slope and I would
rather avoid that route as long as there is a working alternative.

[asgerf, 2015-06-11 15:24:28]

PTAL at patch #3.

Now built-in operators are propagated as pure expressions, although this
propagation was often too aggressive, since it made other assignments unable to
propagate.

I implemented the heuristic that an assignment will not propagate as a pure
expression if doing so would prevent another assignment from propagating.

[Kevin Millikin (Google), 2015-06-12 12:19:10]

This LGTM.

https://codereview.chromium.org/1175973005/diff/40001/pkg/compiler/lib/src/cp...
File pkg/compiler/lib/src/cps_ir/cps_ir_nodes_sexpr.dart (right):

https://codereview.chromium.org/1175973005/diff/40001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_nodes_sexpr.dart:328: return '(TypeExpression
${node.dartType.toString()} ($args))';
Since you're changing it, get rid of toString()?

https://codereview.chromium.org/1175973005/diff/40001/pkg/compiler/lib/src/cp...
File pkg/compiler/lib/src/cps_ir/type_propagation.dart (right):

https://codereview.chromium.org/1175973005/diff/40001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/type_propagation.dart:81: return new
TypeMask.nonNullExact(element, classWorld);
I'm worried that exact means one thin in class TypeMask and another in class
TypeMaskSystem.  Should we change the name of this method to nonNullExact?

https://codereview.chromium.org/1175973005/diff/40001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/type_propagation.dart:450: invoke.parent = letPrim;
Is it necessary to set parent here?  I'd remove it because it seems misleading
(the primitive and invoke argument parents are not set, so something like
replaceSubtree is necessary anyway to create all the parent pointers).

https://codereview.chromium.org/1175973005/diff/40001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/type_propagation.dart:510: if (use is! IsTrue) {
IsTrue ==> Condition, though one might wonder why there is an abstract class
with only one implementation.

https://codereview.chromium.org/1175973005/diff/40001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/type_propagation.dart:520: return new
ApplyBuiltinOperator(operator, <Primitive>[left, right]);
I'm not sure this function is pulling it's weight.  I'd just inline it at its
use site.  Same with the next one.

https://codereview.chromium.org/1175973005/diff/40001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/type_propagation.dart:1287: bool get isNullable =>
kind != NOTHING && type.isNullable;
This predicate and its negation is not monotone, so we should be careful.  Maybe
it needs a comment noting that and mentioning that we use it after analysis has
converged.

https://codereview.chromium.org/1175973005/diff/40001/pkg/compiler/lib/src/tr...
File pkg/compiler/lib/src/tree_ir/optimization/logical_rewriter.dart (right):

https://codereview.chromium.org/1175973005/diff/40001/pkg/compiler/lib/src/tr...
pkg/compiler/lib/src/tree_ir/optimization/logical_rewriter.dart:214: e is
ApplyBuiltinOperator && operatorReturnsBool(e.operator);
I like parens around the && subexpression.  I usually assume the reader will
know the precedence of && and ||, but I don't assume that they will know that I
know it.

It helps with adding another disjunct too, because you can more easily see where
to put it.

https://codereview.chromium.org/1175973005/diff/40001/pkg/compiler/lib/src/tr...
File pkg/compiler/lib/src/tree_ir/optimization/statement_rewriter.dart (right):

https://codereview.chromium.org/1175973005/diff/40001/pkg/compiler/lib/src/tr...
pkg/compiler/lib/src/tree_ir/optimization/statement_rewriter.dart:267: /// True
if [exp] contains a use of a variable that was assigned to by the
"a variable" ==> "the variable" seems more clear.  Also, you mean that the
right-hand side of the assignment was impure (not the assignment itself)?

https://codereview.chromium.org/1175973005/diff/40001/pkg/compiler/lib/src/tr...
pkg/compiler/lib/src/tree_ir/optimization/statement_rewriter.dart:1024: /// Note
that this visitor is only applied to expressions where all
You might mention this as well (that it's cheap) at the use of this
visitor---I'd probably wonder about it when I saw the use, not the definition,
and I don't want to have to scroll down to find out :)

[asgerf, 2015-06-15 09:26:18]

https://codereview.chromium.org/1175973005/diff/40001/pkg/compiler/lib/src/cp...
File pkg/compiler/lib/src/cps_ir/cps_ir_nodes_sexpr.dart (right):

https://codereview.chromium.org/1175973005/diff/40001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_nodes_sexpr.dart:328: return '(TypeExpression
${node.dartType.toString()} ($args))';
On 2015/06/12 12:19:10, Kevin Millikin (Google) wrote:
> Since you're changing it, get rid of toString()?

Done.

https://codereview.chromium.org/1175973005/diff/40001/pkg/compiler/lib/src/cp...
File pkg/compiler/lib/src/cps_ir/type_propagation.dart (right):

https://codereview.chromium.org/1175973005/diff/40001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/type_propagation.dart:81: return new
TypeMask.nonNullExact(element, classWorld);
On 2015/06/12 12:19:10, Kevin Millikin (Google) wrote:
> I'm worried that exact means one thin in class TypeMask and another in class
> TypeMaskSystem.  Should we change the name of this method to nonNullExact?

Absolutely. I meant to do that but then forgot about it.

https://codereview.chromium.org/1175973005/diff/40001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/type_propagation.dart:450: invoke.parent = letPrim;
On 2015/06/12 12:19:10, Kevin Millikin (Google) wrote:
> Is it necessary to set parent here?  I'd remove it because it seems misleading
> (the primitive and invoke argument parents are not set, so something like
> replaceSubtree is necessary anyway to create all the parent pointers).

Nope, you are right. Removed it.

https://codereview.chromium.org/1175973005/diff/40001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/type_propagation.dart:510: if (use is! IsTrue) {
On 2015/06/12 12:19:10, Kevin Millikin (Google) wrote:
> IsTrue ==> Condition, though one might wonder why there is an abstract class
> with only one implementation.

Well it's actually the ToBoolean operation we care about, not the fact that it's
a condition. IsTrue was just the only one that did that, though come to think of
it, the new IsFalsy operator does it too.

I've renamed the method and added IsFalsy as a case (though I haven't seen that
case hit anything yet).

https://codereview.chromium.org/1175973005/diff/40001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/type_propagation.dart:520: return new
ApplyBuiltinOperator(operator, <Primitive>[left, right]);
On 2015/06/12 12:19:10, Kevin Millikin (Google) wrote:
> I'm not sure this function is pulling it's weight.  I'd just inline it at its
> use site.  Same with the next one.

Done.

https://codereview.chromium.org/1175973005/diff/40001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/type_propagation.dart:1287: bool get isNullable =>
kind != NOTHING && type.isNullable;
On 2015/06/12 12:19:10, Kevin Millikin (Google) wrote:
> This predicate and its negation is not monotone, so we should be careful. 
Maybe
> it needs a comment noting that and mentioning that we use it after analysis
has
> converged.

Thanks. isDefinitelyNotNull was a bug. I've fixed it.

isNullable is looks monotone to me (false being the precise answer and true the
safe answer).

isNullConstant is intended as a pattern matching thing, not a lattice query, so
I grouped it with the other pattern matching getters.

https://codereview.chromium.org/1175973005/diff/40001/pkg/compiler/lib/src/tr...
File pkg/compiler/lib/src/tree_ir/optimization/logical_rewriter.dart (right):

https://codereview.chromium.org/1175973005/diff/40001/pkg/compiler/lib/src/tr...
pkg/compiler/lib/src/tree_ir/optimization/logical_rewriter.dart:214: e is
ApplyBuiltinOperator && operatorReturnsBool(e.operator);
On 2015/06/12 12:19:10, Kevin Millikin (Google) wrote:
> I like parens around the && subexpression.  I usually assume the reader will
> know the precedence of && and ||, but I don't assume that they will know that
I
> know it.
> 
> It helps with adding another disjunct too, because you can more easily see
where
> to put it.

What if we just split the line like this? Then the line-breaking align with
precedence.

https://codereview.chromium.org/1175973005/diff/40001/pkg/compiler/lib/src/tr...
File pkg/compiler/lib/src/tree_ir/optimization/statement_rewriter.dart (right):

https://codereview.chromium.org/1175973005/diff/40001/pkg/compiler/lib/src/tr...
pkg/compiler/lib/src/tree_ir/optimization/statement_rewriter.dart:267: /// True
if [exp] contains a use of a variable that was assigned to by the
On 2015/06/12 12:19:10, Kevin Millikin (Google) wrote:
> "a variable" ==> "the variable" seems more clear.  Also, you mean that the
> right-hand side of the assignment was impure (not the assignment itself)?

Not all expressions assign to a variable, so "the" variable is also a bit
misleading.

And yes, for assignments, I mean the right-hand side. I've extended the comment
to clarify.

I also updated the comment on the environment field to indicate that it does not
contain constant assignments.

https://codereview.chromium.org/1175973005/diff/40001/pkg/compiler/lib/src/tr...
pkg/compiler/lib/src/tree_ir/optimization/statement_rewriter.dart:1024: /// Note
that this visitor is only applied to expressions where all
On 2015/06/12 12:19:10, Kevin Millikin (Google) wrote:
> You might mention this as well (that it's cheap) at the use of this
> visitor---I'd probably wonder about it when I saw the use, not the definition,
> and I don't want to have to scroll down to find out :)

Added another comment at the use-site to clarify.

[Kevin Millikin (Google), 2015-06-15 10:24:09]

https://codereview.chromium.org/1175973005/diff/40001/pkg/compiler/lib/src/cp...
File pkg/compiler/lib/src/cps_ir/type_propagation.dart (right):

https://codereview.chromium.org/1175973005/diff/40001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/type_propagation.dart:510: if (use is! IsTrue) {
On 2015/06/15 09:26:18, asgerf wrote:
> On 2015/06/12 12:19:10, Kevin Millikin (Google) wrote:
> > IsTrue ==> Condition, though one might wonder why there is an abstract class
> > with only one implementation.
> 
> Well it's actually the ToBoolean operation we care about, not the fact that
it's
> a condition. IsTrue was just the only one that did that, though come to think
of
> it, the new IsFalsy operator does it too.
> 
> I've renamed the method and added IsFalsy as a case (though I haven't seen
that
> case hit anything yet).

Looks good.

https://codereview.chromium.org/1175973005/diff/40001/pkg/compiler/lib/src/tr...
File pkg/compiler/lib/src/tree_ir/optimization/logical_rewriter.dart (right):

https://codereview.chromium.org/1175973005/diff/40001/pkg/compiler/lib/src/tr...
pkg/compiler/lib/src/tree_ir/optimization/logical_rewriter.dart:214: e is
ApplyBuiltinOperator && operatorReturnsBool(e.operator);
On 2015/06/15 09:26:18, asgerf wrote:
> On 2015/06/12 12:19:10, Kevin Millikin (Google) wrote:
> > I like parens around the && subexpression.  I usually assume the reader will
> > know the precedence of && and ||, but I don't assume that they will know
that
> I
> > know it.
> > 
> > It helps with adding another disjunct too, because you can more easily see
> where
> > to put it.
> 
> What if we just split the line like this? Then the line-breaking align with
> precedence.

It's OK this way.

[asgerf, 2015-06-15 15:31:54]

Committed patchset #9 (id:160001) manually as
eec1d88d9cdf77d4c2dd64a4b0e8965dd1040ba3 (presubmit successful).