code review 5671096: spec: make all comparison results untyped bool

spec: make all comparison results untyped bool

Or, depending on your point of view, make the
comparisons satisfy any surrounding boolean type.

Also, fix a few foo_bar -> fooBar in code fragments.

Fixes issue 2561.

[rsc, 2012-02-18 21:10:32]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://code.google.com/p/go/

[r, 2012-02-18 21:19:38]

LGTM
that's one way to fix it

[bradfitz, 2012-02-18 23:28:45]

Update cmd/api in this CL too?  It has a case for ideal-bool.


On Sat, Feb 18, 2012 at 1:10 PM, <rsc@golang.org> wrote:

> Reviewers: golang-dev_googlegroups.com,
>
> Message:
> Hello golang-dev@googlegroups.com,
>
> I'd like you to review this change to
> https://code.google.com/p/go/
>
>
> Description:
> spec: delete untyped bool idea
>
> The spec and compilers were inconsistent about which
> boolean expressions were 'ideal bool' and which were
> the predefined named type bool.  Unlike the arithmetic
> types, bools can arise from operations that do not begin
> with bools, so there is no context to help decide the type.
>
> An alternative would be to define that all boolean expressions,
> even non-constant ones, have a type inferred from the
> surrounding context, in the same way that non-constant
> shift expressions do.  However, non-constant shift expressions
> have been such a rich source of bugs that it seems like a
> mistake to do that a second time.
>
> Also, fix a few foo_bar -> fooBar in code fragments.
>
> Implementation in
http://codereview.appspot.com/**5674098<http://codereview.appspot.com/5674098>
> .
> Only tests are affected.
>
> Fixes issue 2561.
>
> Please review this at
http://codereview.appspot.com/**5671096/<http://codereview.appspot.com/5671096/>
>
> Affected files:
>  M doc/go_spec.html
>
>
> Index: doc/go_spec.html
> ==============================**==============================**=======
> --- a/doc/go_spec.html
> +++ b/doc/go_spec.html
> @@ -2238,7 +2238,7 @@
>
>  <pre>
>  f := func(x, y int) int { return x + y }
> -func(ch chan int) { ch &lt;- ACK } (reply_chan)
> +func(ch chan int) { ch &lt;- ACK }(replyChan)
>  </pre>
>
>  <p>
> @@ -2827,7 +2827,7 @@
>  x &lt;= f()
>  ^a &gt;&gt; b
>  f() || g()
> -x == y+1 &amp;&amp; &lt;-chan_ptr &gt; 0
> +x == y+1 &amp;&amp; &lt;-chanPtr &gt; 0
>  </pre>
>
>
> @@ -3507,8 +3507,8 @@
>  </p>
>
>  <p>
> -Untyped boolean, numeric, and string constants may be used as operands
> -wherever it is legal to use an operand of boolean, numeric, or string
> type,
> +Untyped numeric, and string constants may be used as operands
> +wherever it is legal to use an operand of numeric or string type,
>  respectively.
>  Except for shift operations, if the operands of a binary operation are
>  different kinds of untyped constants, the operation and result use
> @@ -3518,8 +3518,8 @@
>  </p>
>
>  <p>
> -A constant <a href="#Comparison_operators">**comparison</a> always yields
> -an untyped boolean constant. If the left operand of a constant
> +Boolean constants and constant <a href="#Comparison_operators">**
> comparisons</a>
> +have type <code>bool</code>.  If the left operand of a constant
>  <a href="#Operators">shift expression</a> is an untyped constant, the
>  result is an integer constant; otherwise it is a constant of the same
>  type as the left operand, which must be of integer type
> @@ -3539,7 +3539,7 @@
>  const f = int32(1) &lt;&lt; 33   // f == 0     (type int32)
>  const g = float64(2) &gt;&gt; 1  // illegal    (float64(2) is a typed
> floating-point constant)
>  const h = "foo" &gt; "bar"    // h == true  (untyped boolean constant)
> -const j = true             // j == true  (untyped boolean constant)
> +const j = true             // j == true  (type bool)
>  const k = 'w' + 1          // k == 'x'   (untyped character constant)
>  const l = "hi"             // l == "hi"  (untyped string constant)
>  const m = string(k)        // m == "x"   (type string)
> @@ -3864,9 +3864,9 @@
>  <a href="#Assignability">**assignable</a> to the type of the
>  operand to which it is assigned. If an untyped <a
> href="#Constants">constant</a>
>  is assigned to a variable of interface type, the constant is <a
> href="#Conversions">converted<**/a>
> -to type <code>bool</code>, <code>rune</code>, <code>int</code>,
> <code>float64</code>,
> +to type <code>rune</code>, <code>int</code>, <code>float64</code>,
>  <code>complex128</code> or <code>string</code>
> -respectively, depending on whether the value is a boolean,
> +respectively, depending on whether the value is a
>  character, integer, floating-point, complex, or string constant.
>  </p>
>
> @@ -4049,16 +4049,16 @@
>  v := x  // x is evaluated exactly once
>  if v == nil {
>        printString("x is nil")
> -} else if i, is_int := v.(int); is_int {
> +} else if i, isInt := v.(int); isInt {
>        printInt(i)  // i is an int
> -} else if i, is_float64 := v.(float64); is_float64 {
> +} else if i, isFloat64 := v.(float64); isFloat64 {
>        printFloat64(i)  // i is a float64
> -} else if i, is_func := v.(func(int) float64); is_func {
> +} else if i, isFunc := v.(func(int) float64); isFunc {
>        printFunction(i)  // i is a function
>  } else {
> -       i1, is_bool := v.(bool)
> -       i2, is_string := v.(string)
> -       if is_bool || is_string {
> +       i1, isBool := v.(bool)
> +       i2, isString := v.(string)
> +       if isBool || isString {
>                i := v
>                printString("type is bool or string")  // i is an
> interface{}
>        } else {
> @@ -4388,7 +4388,7 @@
>  specify any result values.
>  </p>
>  <pre>
> -func no_result() {
> +func noResult() {
>        return
>  }
>  </pre>
> @@ -4404,11 +4404,11 @@
>                and <a href="#Assignability">**assignable</a>
>                to the corresponding element of the function's result type.
>  <pre>
> -func simple_f() int {
> +func simpleF() int {
>        return 2
>  }
>
> -func complex_f1() (re float64, im float64) {
> +func complexF1() (re float64, im float64) {
>        return -7.0, -4.0
>  }
>  </pre>
> @@ -4420,8 +4420,8 @@
>                "return" statement listing these variables, at which point
> the
>                rules of the previous case apply.
>  <pre>
> -func complex_f2() (re float64, im float64) {
> -       return complex_f1()
> +func complexF2() (re float64, im float64) {
> +       return complexF1()
>  }
>  </pre>
>        </li>
> @@ -4431,7 +4431,7 @@
>                and the function may assign values to them as necessary.
>                The "return" statement returns the values of these
> variables.
>  <pre>
> -func complex_f3() (re float64, im float64) {
> +func complexF3() (re float64, im float64) {
>        re = 7.0
>        im = 4.0
>        return
>
>
>

[rsc, 2012-02-18 23:30:06]

I'll send a different CL for cmd/api.

[bradfitz, 2012-02-18 23:31:04]

http://codereview.appspot.com/5671096/diff/3001/doc/go_spec.html
File doc/go_spec.html (right):

http://codereview.appspot.com/5671096/diff/3001/doc/go_spec.html#newcode2241
doc/go_spec.html:2241: func(ch chan int) { ch <- ACK }(replyChan)
all caps ACK variable/constant isn't really Go style either, if that's what
you're fixing.

http://codereview.appspot.com/5671096/diff/3001/doc/go_spec.html#newcode3510
doc/go_spec.html:3510: Untyped numeric, and string constants may be used as
operands
s/numeric,/numeric/ ?

[gri, 2012-02-21 19:46:48]

It seems to me that this change is making bools more unusual than all the other
types and adding an irregularity. Instead, one could just have said that boolean
values resulting from comparison always yield an ideal bool no matter what
operands are involved (I actually thought that spec said that at some time in
the past).

After all, such bools are made from "whole cloth" and arguably are untyped. The
shift issue doesn't come in here. It would be a smaller spec change. Now we have
cases where we need to introduce casts where there were none before.

http://codereview.appspot.com/5671096/diff/3001/doc/go_spec.html
File doc/go_spec.html (right):

http://codereview.appspot.com/5671096/diff/3001/doc/go_spec.html#newcode3
doc/go_spec.html:3: "Subtitle": "Version of February 16, 2012"
up-date before submitting

[rsc, 2012-02-21 20:02:43]

On Tue, Feb 21, 2012 at 14:46,  <gri@golang.org> wrote:
> Instead, one could just have
> said that boolean values resulting from comparison always yield an ideal
> bool no matter what operands are involved (I actually thought that spec
> said that at some time in the past).

In fact I am going to write that text today.

[rsc, 2012-02-21 20:41:29]

Hello golang-dev@googlegroups.com, r@golang.org, bradfitz@golang.org,
gri@golang.org (cc: golang-dev@googlegroups.com),

Please take another look.

[rsc, 2012-02-21 20:42:27]

This new version of the change should be less intrusive.
I will do the compiler fixes next.

[r, 2012-02-21 21:03:49]

http://codereview.appspot.com/5671096/diff/8001/doc/go_spec.html
File doc/go_spec.html (right):

http://codereview.appspot.com/5671096/diff/8001/doc/go_spec.html#newcode3095
doc/go_spec.html:3095: type Bool bool
s/Bool/MyBool/ or /Truth/ or something more distinct from "bool"

http://codereview.appspot.com/5671096/diff/8001/doc/go_spec.html#newcode3097
doc/go_spec.html:3097: func test2(bool)
variables of type bool and whateveryouchoose would be clearer than the implicit
assignment to a function parameter for an unimplemented function

[rsc, 2012-02-21 21:13:39]

PTAL

[r, 2012-02-21 21:15:20]

LGTM

[gri, 2012-02-21 21:28:56]

I like this change much better.

Should we make the analogous change for ,ok operations? Not required, but seems
like it's in the same vain.

http://codereview.appspot.com/5671096/diff/8001/doc/go_spec.html
File doc/go_spec.html (right):

http://codereview.appspot.com/5671096/diff/8001/doc/go_spec.html#newcode2595
doc/go_spec.html:2595: the result of the assertion is a pair of values with
types <code>(T, bool)</code>.
Should this also return an untyped boolean now? It is a form of "comparison". In
the "v, ok :=" case, ok will be bool, but in "v, ok = " ok could be another
boolean type.

http://codereview.appspot.com/5671096/diff/8001/doc/go_spec.html#newcode2980
doc/go_spec.html:2980: Comparison operators compare two operands and yield a
boolean value.
maybe: "untyped boolean value" to be more explicit?

http://codereview.appspot.com/5671096/diff/8001/doc/go_spec.html#newcode3100
doc/go_spec.html:3100: test(x == y)  // ok; result of comparison has type Bool
leave away "ok;" ?

In other examples we usually only point out illegal examples with "Illegal",
otherwise we assume that the example is ok w/o saying it explicitly

http://codereview.appspot.com/5671096/diff/8001/doc/go_spec.html#newcode3177
doc/go_spec.html:3177: The boolean variable <code>ok</code> indicates whether
same here: bool or untyped boolean? It should be the same as for type
assertions.

[rsc, 2012-02-21 21:35:44]

I am inclined to leave ,ok alone.  It has not been a problem.

[gri, 2012-02-21 21:52:54]

LGTM

http://codereview.appspot.com/5671096/diff/11001/doc/go_spec.html
File doc/go_spec.html (right):

http://codereview.appspot.com/5671096/diff/11001/doc/go_spec.html#newcode3
doc/go_spec.html:3: "Subtitle": "Version of February 16, 2012"
up-date

http://codereview.appspot.com/5671096/diff/11001/doc/go_spec.html#newcode3099
doc/go_spec.html:3099: b1 MyBool = x == y // ok; result of comparison has type
MyBool
s/ok; //

[rsc, 2012-02-21 22:29:21]

done

> http://codereview.appspot.com/5671096/diff/11001/doc/go_spec.html#newcode3
> doc/go_spec.html:3: "Subtitle": "Version of February 16, 2012"
> up-date
>
> http://codereview.appspot.com/5671096/diff/11001/doc/go_spec.html#newcode3099
> doc/go_spec.html:3099: b1 MyBool = x == y // ok; result of comparison
> has type MyBool
> s/ok; //
>
> http://codereview.appspot.com/5671096/

[iant, 2012-02-22 01:31:17]

LGTM

http://codereview.appspot.com/5671096/diff/11004/doc/go_spec.html
File doc/go_spec.html (right):

http://codereview.appspot.com/5671096/diff/11004/doc/go_spec.html#newcode3089
doc/go_spec.html:3089: The result of a comparison can assigned to any boolean
type.
s/can assigned/can be assigned/

http://codereview.appspot.com/5671096/diff/11004/doc/go_spec.html#newcode3528
doc/go_spec.html:3528: wherever it is legal to use an operand of boolean,
numeric or string type,
The spec usually does have a comma before "or" or "and" in a list, so this comma
should probably be kept.

[kevlar, 2012-02-22 01:35:53]

http://codereview.appspot.com/5671096/diff/11004/doc/go_spec.html
File doc/go_spec.html (right):

http://codereview.appspot.com/5671096/diff/11004/doc/go_spec.html#newcode3531
doc/go_spec.html:3531: different kinds of untyped constants, the operation and,
for non-boolean operations, the result use
use (for non-boolean operations) to simplify this hard-to-parse sentence?

[rsc, 2012-02-22 03:03:25]

On Tue, Feb 21, 2012 at 20:35,  <kevlar@google.com> wrote:
> operation and, for non-boolean operations, the result use
> use (for non-boolean operations) to simplify this hard-to-parse
> sentence?

parentheticals are usually reserved for optional text.
this text is needed for correctness.

[rsc, 2012-02-22 03:04:39]

*** Submitted as http://code.google.com/p/go/source/detail?r=61e30316d672 ***

spec: make all comparison results untyped bool

Or, depending on your point of view, make the
comparisons satisfy any surrounding boolean type.

Also, fix a few foo_bar -> fooBar in code fragments.

Fixes issue 2561.

R=golang-dev, r, bradfitz, gri, iant, kevlar
CC=golang-dev
http://codereview.appspot.com/5671096

[rsc, 2012-02-22 03:12:36]

On Tue, Feb 21, 2012 at 20:31,  <iant@golang.org> wrote:
> The spec usually does have a comma before "or" or "and" in a list, so
> this comma should probably be kept.

done.  really i just forgot to put it back; not intentional.