|
|
Descriptionspec: && and || permit operands of different boolean types
If the operand types differ (newly permitted), the result
is an untyped boolean value. Otherwise the result type is
the same as the (typed) operand's type.
This is a backward-compatible language change.
The actual change is in the section for logical operators.
However, it required adjustments to the section on general
operators. The text in that section would now more naturally
fit into the sections for the different operators. Leaving
that cleanup for a separate CL to keep this one simple.
Fixes issue 4336.
Patch Set 1 #Patch Set 2 : diff -r 4a3d328fe8e9 https://code.google.com/p/go #Patch Set 3 : diff -r 4a3d328fe8e9 https://code.google.com/p/go #
Total comments: 2
Patch Set 4 : diff -r 4a3d328fe8e9 https://code.google.com/p/go #MessagesTotal messages: 17
Hello r@golang.org, rsc@golang.org, iant@golang.org, ken@golang.org (cc: golang-dev@googlegroups.com), I'd like you to review this change to https://code.google.com/p/go
Sign in to reply to this message.
this is easy but it seems odd to have a special case like this. what's the reasoning?
Sign in to reply to this message.
OK. Here is the piece of reasoning, from the issue: I think we should fix this in the following [way]: x && y is equivalent to x == true && y == true (same for ||, of course) and the result of && is an untyped bool. It still feels like a special case but at least I see a justification now.
Sign in to reply to this message.
Also, since the result may depend on the value of the first operand only, arguably the two shouldn't be forced to have the same type. Even the spec explains them as control flow with the if expression. I think it will clean things up in the long run. We can group binary operators in the following classes: comparisons arithmetic operators (excl. shifts) shift operators logical operators Until now we have more or less tried to all keep them under one roof w/ exceptions here and there. It's easier to separate them out and have specific (and different) rules for each set. Now only arithmetic operators require operands to have identical types, and that's the one place where we really care about it. - gri On Fri, Aug 2, 2013 at 3:42 PM, Rob Pike <r@golang.org> wrote: > OK. Here is the piece of reasoning, from the issue: > > I think we should fix this in the following [way]: > > x && y is equivalent to x == true && y == true (same for ||, of course) > > and the result of && is an untyped bool. > > > It still feels like a special case but at least I see a justification now. >
Sign in to reply to this message.
LGTM but wait for others
Sign in to reply to this message.
https://codereview.appspot.com/12382043/diff/6001/doc/go_spec.html File doc/go_spec.html (right): https://codereview.appspot.com/12382043/diff/6001/doc/go_spec.html#newcode2922 doc/go_spec.html:2922: For <a href="#Arithmetic_operators">arithmetic operators</a>, the operand I wonder if we should move the discussion of types to the Arithmetic operators section, just as the discussion of types for comparisons is in the Comparison operators section.
Sign in to reply to this message.
https://codereview.appspot.com/12382043/diff/6001/doc/go_spec.html File doc/go_spec.html (right): https://codereview.appspot.com/12382043/diff/6001/doc/go_spec.html#newcode2922 doc/go_spec.html:2922: For <a href="#Arithmetic_operators">arithmetic operators</a>, the operand On 2013/08/02 23:51:41, iant wrote: > I wonder if we should move the discussion of types to the Arithmetic operators > section, just as the discussion of types for comparisons is in the Comparison > operators section. Yes, but (per CL description) I prefer doing this in a separate CL which does not change the language.
Sign in to reply to this message.
LGTM
Sign in to reply to this message.
(Sorry I didn't see this earlier.) I'm not convinced this is a backwards-compatible change. Consider: type B bool var b B var r interface{} := b || b _ = r.(B) Under the current spec, this program terminates normally; under the new spec, it panics because r contains a bool, not a B. In fact, this exact problem bit me today since ssa had implemented (approximately) the proposed behaviour and this caused mis-analysis of the code in file go.tools/go/exact.go, func BinaryOp, where two exact.boolVals are ||'d or &&'d together and the result is put in an exact.Value (interface{}) on the assumption that it too is a boolVal. That code will be broken by the proposed change. FWIW, I don't have a strong feeling for or against this change; I just want to make sure we haven't forgotten anything. cheers alan On 2 August 2013 21:15, <rsc@golang.org> wrote: > LGTM > > > https://codereview.appspot.**com/12382043/<https://codereview.appspot.com/123... > > -- > > ---You received this message because you are subscribed to the Google > Groups "golang-dev" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to golang-dev+unsubscribe@**googlegroups.com<golang-dev%2Bunsubscribe@googlegrou... > . > For more options, visit https://groups.google.com/**groups/opt_out<https://groups.google.com/groups/o... > . > > >
Sign in to reply to this message.
I guess we should say that if the types of both sides agree, the result has that type; otherwise the result is an untyped bool. That is backwards compatible, since the types were required to agree before.
Sign in to reply to this message.
Sigh... I will send out an updated version later this weekend. - gri On Fri, Aug 2, 2013 at 8:17 PM, Russ Cox <rsc@golang.org> wrote: > I guess we should say that if the types of both sides agree, the result > has that type; otherwise the result is an untyped bool. That is backwards > compatible, since the types were required to agree before. > > >
Sign in to reply to this message.
Hello r@golang.org, rsc@golang.org, iant@golang.org, ken@golang.org, adonovan@google.com (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
PS: I am not so strongly convinced anymore that this change is better than what we have now: Now the rules are actually more complicated than before. Note the "typed operands" trick in the phrasing to keep it simple. Better suggestions welcome. - gri On Fri, Aug 2, 2013 at 10:53 PM, <gri@golang.org> wrote: > Hello r@golang.org, rsc@golang.org, iant@golang.org, ken@golang.org, > adonovan@google.com (cc: golang-dev@googlegroups.com), > > Please take another look. > > > https://codereview.appspot.**com/12382043/<https://codereview.appspot.com/123... >
Sign in to reply to this message.
Replacing golang-dev with golang-codereviews.
Sign in to reply to this message.
R=close assuming this is stale
Sign in to reply to this message.
Yes, the corresponding issue 4336 would amount to a backward-incompatible language change. Postponed indefinitely. - gri On Wed, Mar 5, 2014 at 11:40 AM, <rsc@golang.org> wrote: > R=close > > assuming this is stale > > > https://codereview.appspot.com/12382043/ >
Sign in to reply to this message.
|