code review 171760043: compiler: Logical operators should yield untyped bool f...

compiler: Logical operators should yield untyped bool for untyped operands. Fixes issue 6671 for gccgo.

[cmang, 2014-10-31 20:57:54]

Hello iant@golang.org (cc: gofrontend-dev@googlegroups.com, gri@golang.org), I'd like you to review this change to https://code.google.com/p/gofrontend

[iant, 2014-10-31 23:15:19]

https://codereview.appspot.com/171760043/diff/40001/go/statements.cc File go/statements.cc (right): https://codereview.appspot.com/171760043/diff/40001/go/statements.cc#newcode3849 go/statements.cc:3849: Statement::make_temporary(val->type(), val, loc); Why do you need this change? val->type() is the default type anyhow. What is the effect of passing it explicitly?

[cmang, 2014-11-03 22:42:11]

I think this approach is simpler than what I was attempting to do at first. For this issue, the inconsistency is that assignments where the initialization contains logical operations were forced to have a non-abstract context even if both operands were abstract. I thought it'd be better to fix the logic in Binary_expression::do_determine_types and Binary_expression::operation_type, but I think they do the right thing and we should just allow abstract result types for logical binary expressions. PTAL. https://codereview.appspot.com/171760043/diff/40001/go/statements.cc File go/statements.cc (right): https://codereview.appspot.com/171760043/diff/40001/go/statements.cc#newcode3849 go/statements.cc:3849: Statement::make_temporary(val->type(), val, loc); On 2014/10/31 23:15:19, iant wrote: > Why do you need this change? val->type() is the default type anyhow. What is > the effect of passing it explicitly? This is the wrong approach, but it fixed it a failure in bug465.dir's compilation of switch statements. Passing the type worked here because it meant Temporary_statement::do_determine_type would determine the type with a given type context and would resolve to some non-abstract binary type.

[iant, 2014-11-04 14:18:28]

https://codereview.appspot.com/171760043/diff/80001/go/statements.cc File go/statements.cc (right): https://codereview.appspot.com/171760043/diff/80001/go/statements.cc#newcode567 go/statements.cc:567: may_be_abstract = true; This doesn't feel right to me. It's not really the case that it's OK for the type of the expression to be abstract. What we want is that the type of && or || should become the type of the variable rather than the named type bool. There is this code in Binary_expression::do_determine_type: if (this->op_ == OPERATOR_ANDAND || this->op_ == OPERATOR_OROR) { // For a logical operation, the context does not determine the // types of the operands. The operands must be some boolean // type but if the context has a boolean type they do not // inherit it. See http://golang.org/issue/3924. subcontext.type = NULL; } Looking at issue 3924, I see that this issue (6671) is essentially contradicting 3924. That is, we've decided that 3924 is wrong. So I think we should remove that block of code. If we do that, the desired type will propagate down to the comparison operators, and the code at the very bottom of Binary_expression::do_determine_type should give the comparison operators the desired type.

[cmang, 2014-11-04 17:10:18]

https://codereview.appspot.com/171760043/diff/80001/go/statements.cc File go/statements.cc (right): https://codereview.appspot.com/171760043/diff/80001/go/statements.cc#newcode567 go/statements.cc:567: may_be_abstract = true; On 2014/11/04 14:18:28, iant wrote: > This doesn't feel right to me. It's not really the case that it's OK for the > type of the expression to be abstract. What we want is that the type of && or > || should become the type of the variable rather than the named type bool. > > There is this code in Binary_expression::do_determine_type: > > if (this->op_ == OPERATOR_ANDAND || this->op_ == OPERATOR_OROR) > { > // For a logical operation, the context does not determine the > // types of the operands. The operands must be some boolean > // type but if the context has a boolean type they do not > // inherit it. See http://golang.org/issue/3924. > subcontext.type = NULL; > } > > Looking at issue 3924, I see that this issue (6671) is essentially contradicting > 3924. That is, we've decided that 3924 is wrong. So I think we should remove > that block of code. If we do that, the desired type will propagate down to the > comparison operators, and the code at the very bottom of > Binary_expression::do_determine_type should give the comparison operators the > desired type. Done.

[iant, 2014-11-04 17:44:43]

LGTM

[iant, 2014-11-04 17:45:01]

*** Submitted as https://code.google.com/p/gofrontend/source/detail?r=836325fe0181 *** compiler: Logical operators should yield untyped bool for untyped operands. Fixes issue 6671 for gccgo. LGTM=iant R=iant CC=gofrontend-dev, gri https://codereview.appspot.com/171760043 Committer: Ian Lance Taylor <iant@golang.org>