FAQ
Reviewers: golang-dev_googlegroups.com,

Message:
Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com),

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


Description:
cmd/gc: Make sure bools lose idealness when used with logical operators.

Bools from comparisons can be assigned to all bool types, but this
idealness would propagate through logical operators when the result
should have been lowered to a non-ideal form.

Fixes issue 3924.

Please review this at http://codereview.appspot.com/6855061/

Affected files:
M src/cmd/gc/typecheck.c
A test/fixedbugs/issue3924.go


Index: src/cmd/gc/typecheck.c
===================================================================
--- a/src/cmd/gc/typecheck.c
+++ b/src/cmd/gc/typecheck.c
@@ -614,7 +614,10 @@
n->left = l;
n->right = r;
}
- }
+ // non-comparison operators on ideal bools should make them lose their
ideal-ness
+ } else if(t == idealbool)
+ t = types[TBOOL];
+
if(et == TSTRING) {
if(iscmp[n->op]) {
n->etype = n->op;
Index: test/fixedbugs/issue3924.go
===================================================================
new file mode 100644
--- /dev/null
+++ b/test/fixedbugs/issue3924.go
@@ -0,0 +1,13 @@
+// errorcheck
+
+// Copyright 2012 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package foo
+
+type mybool bool
+
+var x, y = 1, 2
+var _ mybool = x < y && x < y // ERROR "cannot use"
+var _ mybool = x < y || x < y // ERROR "cannot use"

Search Discussions

  • Remyoudompheng at Nov 18, 2012 at 11:04 pm
    I don't understand why the comparison result should be lowered to a
    concrete type before being assigned to a variable.

    http://codereview.appspot.com/6855061/
  • Daniel Morsing at Nov 18, 2012 at 11:23 pm

    On 2012/11/18 23:04:27, remyoudompheng wrote:
    I don't understand why the comparison result should be lowered to a concrete
    type before being assigned to a variable.
    && and || are logical operators. Their results are not supposed to be
    ideal. Before this fix, they would become ideal if both operands were.

    You could stretch the wording of the spec a bit and say that the
    idealness is part of the resulting type. I think the wording about
    context means that they should be lowered to a bool when used as both
    operands to a logical operator.



    https://codereview.appspot.com/6855061/
  • Rob Pike at Nov 18, 2012 at 11:35 pm
    Here's the relevant sentence from the spec:

    Logical operators apply to boolean values and yield a result of the
    same type as the operands.

    Given that sentence and the examples in the provided test program,
    this CL looks right to me. I am not qualified to comment on the
    compiler change however.

    -rob
  • Rémy Oudompheng at Nov 19, 2012 at 1:19 am

    On 2012/11/19 Rob Pike wrote:
    Here's the relevant sentence from the spec:

    Logical operators apply to boolean values and yield a result of the
    same type as the operands.

    Given that sentence and the examples in the provided test program,
    this CL looks right to me. I am not qualified to comment on the
    compiler change however.
    Then I am confused by the subtle distinction between:
    * untyped non-constant boolean resulting from a comparison
    * untyped constant boolean used in constant expressions.

    In the first case, using a logical operator will coerce the operands
    to their typed flavours. In the second case, the result of a logical
    operation remains an untyped boolean.

    It feels like that distinction introduces a notion of untyped
    non-constant value, which doesn't exist elsewhere in the spec. Maybe I
    just have a twisted mind and I should rather think that the result of
    a comparison is "bool except when it is immediately assigned to a
    variable of any boolean type, in which case the assignment is legal".

    In the sentence "The result of a comparison can be assigned to any
    boolean type. If the context does not demand a specific boolean type,
    the result has type bool.", the word context is vague and can suggest
    the code in the CL test case is valid.

    Rémy.
  • Russ Cox at Nov 19, 2012 at 1:36 pm
    I am quite confused about whether this is the behavior we want. The
    distinction about which computed bools are 'ideal' and which are not
    seems too unpredictable. There are other open issues about bools too.
    I wonder if we should reexamine what the rules are.

    Russ
  • Michael Jones at Nov 19, 2012 at 2:01 pm
    ...and maybe add the missing boolean operations & and | friends?
    On Mon, Nov 19, 2012 at 5:31 AM, Russ Cox wrote:

    I am quite confused about whether this is the behavior we want. The
    distinction about which computed bools are 'ideal' and which are not
    seems too unpredictable. There are other open issues about bools too.
    I wonder if we should reexamine what the rules are.

    Russ


    --
    Michael T. Jones | Chief Technology Advocate | mtj@google.com | +1
    650-335-5765
  • Daniel Morsing at Nov 20, 2012 at 6:02 pm

    On Mon, Nov 19, 2012 at 2:31 PM, Russ Cox wrote:
    I am quite confused about whether this is the behavior we want. The
    distinction about which computed bools are 'ideal' and which are not
    seems too unpredictable. There are other open issues about bools too.
    I wonder if we should reexamine what the rules are.

    Russ
    Would there be any adverse effects by defining logical operations as
    having the convertible bool result? I'm struggling to see where that
    would be a problem.
  • Remyoudompheng at Nov 20, 2012 at 5:52 pm

    On 2012/11/20 16:09:01, DMorsing wrote:
    On Mon, Nov 19, 2012 at 2:31 PM, Russ Cox
    wrote:
    I am quite confused about whether this is the behavior we want. The
    distinction about which computed bools are 'ideal' and which are not
    seems too unpredictable. There are other open issues about bools
    too.
    I wonder if we should reexamine what the rules are.

    Russ
    Would there be any adverse effects by defining logical operations as
    having the convertible bool result? I'm struggling to see where that
    would be a problem.
    Yes: if a and b have type MyBool, a && b should have type MyBool. I
    would tend to have the following rules.

    A boolean value may be typed or untyped.
    * An untyped boolean constant is untyped.
    * The result of a comparison (==, !=, <=, >=, <, >) is untyped.

    A boolean value is assignable to type T:
    * if T is an interface type. If the value is untyped, it is converted to
    bool.
    * if T is a boolean type and the value is untyped.
    * if T is a boolean type and the value has type T.

    Declaration-assignment of a variable with an untyped boolean value has
    type bool.

    The result of a logical operation (&&, ||, !):
    * has type T if one of its operands has type T. The operation is illegal
    if the other operand is not assignable to T.
    * is untyped when both operands are untyped

    Something I don't like with these rules is that if a and b have boolean
    type T1, and T2 is a boolean type distinct from T1, a == b and a != b
    are assignable to T2. I would naively interpret a != b as "a XOR b" and
    it would be a typed value of type T1. But I prefer to have consistency
    with numeric types and arithmetic operators.

    http://codereview.appspot.com/6855061/
  • Daniel Morsing at Nov 20, 2012 at 5:58 pm

    On 2012/11/20 17:30:45, remyoudompheng wrote:
    On 2012/11/20 16:09:01, DMorsing wrote:
    On Mon, Nov 19, 2012 at 2:31 PM, Russ Cox
    wrote:
    I am quite confused about whether this is the behavior we want.
    The
    distinction about which computed bools are 'ideal' and which are
    not
    seems too unpredictable. There are other open issues about bools
    too.
    I wonder if we should reexamine what the rules are.

    Russ
    Would there be any adverse effects by defining logical operations as
    having the convertible bool result? I'm struggling to see where that
    would be a problem.
    Yes: if a and b have type MyBool, a && b should have type MyBool. I
    would tend
    to have the following rules.
    This is the status quo for gc. All that's needed is a clarification in
    the specification.

    However, I don't think we need to add the concept of "untyped bool" to
    the spec. I'd much rather have the current wording (results from
    comparisons can be assigned to any bool type) and add a caveat in the
    section about logical operators. Something to the tune of "If both
    operands are results from comparisons, the result of the logical
    operator is assignable to any bool type".

    We still need to figure out what the situation is with gccgo. I'm not
    sure how consistent the 2 implementations are with how they handle
    booleans.

    https://codereview.appspot.com/6855061/
  • Russ Cox at Nov 26, 2012 at 7:07 pm
    LGTM

    I think the spec might be helped by some tweaks, but this matches the
    spec so it should go in.
  • Daniel Morsing at Nov 26, 2012 at 9:46 pm

    On Mon, Nov 26, 2012 at 8:01 PM, Russ Cox wrote:
    LGTM

    I think the spec might be helped by some tweaks, but this matches the
    spec so it should go in.
    I'm committing this for now, but I think we need to reevaluate the
    handling of bools. Right now the spec is vague, and the
    implementations are inconsistent on how they handle them.
  • Russ Cox at Nov 26, 2012 at 10:09 pm
    I thought this CL was making gc consistent with gccgo?
    What's inconsistent still?
  • Daniel Morsing at Nov 26, 2012 at 9:21 pm

    On Mon, Nov 26, 2012 at 10:16 PM, Russ Cox wrote:
    I thought this CL was making gc consistent with gccgo?
    What's inconsistent still?
    Issue 3923 and 3915
  • Daniel Morsing at Nov 26, 2012 at 9:23 pm
    *** Submitted as
    http://code.google.com/p/go/source/detail?r=81b14ef2226a ***

    cmd/gc: Make sure bools lose idealness when used with logical operators.

    Bools from comparisons can be assigned to all bool types, but this
    idealness would propagate through logical operators when the result
    should have been lowered to a non-ideal form.

    Fixes issue 3924.

    R=golang-dev, remyoudompheng, r, rsc, mtj
    CC=golang-dev
    http://codereview.appspot.com/6855061


    http://codereview.appspot.com/6855061/

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
groupgolang-dev @
categoriesgo
postedNov 18, '12 at 6:16p
activeNov 26, '12 at 10:09p
posts15
users5
websitegolang.org

People

Translate

site design / logo © 2022 Grokbase