code review 7861045: test: more systematic shift tests

test: more systematic shift tests

To be submitted once gc agrees.

[gri, 2013-03-15 22:51:17]

Hello rsc@golang.org, iant@golang.org (cc: golang-dev@googlegroups.com),

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

[remyoudompheng, 2013-03-15 23:43:45]

https://codereview.appspot.com/7861045/diff/7001/test/shift1.go
File test/shift1.go (right):

https://codereview.appspot.com/7861045/diff/7001/test/shift1.go#newcode143
test/shift1.go:143: _ = a[1.<<s]
rsc says that this is invalid on issue 4813, you seem to disagree.

I can't manage to say what  to think of it: 1.<<s should be typed as 1. in
a[1.], but indexes can be untyped, what is the type of 1. in a[1.], can you
confirm?

https://codereview.appspot.com/7861045/diff/7001/test/shift1.go#newcode168
test/shift1.go:168: //_ = complex(1.1<<s, 0) // ERROR "shift of type float64"
the crash has been fixed by revision 7861045

[remyoudompheng, 2013-03-15 23:49:39]

https://codereview.appspot.com/7861045/diff/7001/test/shift1.go
File test/shift1.go (right):

https://codereview.appspot.com/7861045/diff/7001/test/shift1.go#newcode153
test/shift1.go:153: _ = float32(1.)       // ERROR "shift of type float32"
I don't think line 153 is an error.

[gri, 2013-03-16 00:38:50]

PTAL

https://codereview.appspot.com/7861045/diff/7001/test/shift1.go
File test/shift1.go (right):

https://codereview.appspot.com/7861045/diff/7001/test/shift1.go#newcode143
test/shift1.go:143: _ = a[1.<<s]
On 2013/03/15 23:43:45, remyoudompheng wrote:
> rsc says that this is invalid on issue 4813, you seem to disagree.
> 
> I can't manage to say what  to think of it: 1.<<s should be typed as 1. in
> a[1.], but indexes can be untyped, what is the type of 1. in a[1.], can you
> confirm?

I think we agreed that a[1.] is valid (at least r, iant, and I think it should
be so).

I understand rsc's comment #13 in issue 4813. I think rsc is correct in the
sense that the spec does not explicitly require a specific type for the index,
and with that argument, the a[1.<<s] would indeed be incorrect (because the
1.<<s defaults to the default type for the untyped constant 1.0 which is
float64, now that the special int rule has gone).

I am not sure that is what we want. The spec is sufficiently clear in saying
that an untyped index value that can be represented as an integer type is ok,
and since 1. satisfies that requirement, 1.<<s does as well, and so this should
be valid.

That said, I think the spec is just not clear enough. Personally I would vote to
make it legal, but it's always easier to leave it illegal and change it later
(the other way round is not possible in a backward compatible way).

I have some ideas to tie this down once and for all in the spec by introducing
the notion of a "context type" (analogous to, but different from the notion of
static and dynamic type): The context type only matters for untyped values and
it is the type such values assume when used in the respective context. The usual
errors are reported if an untyped value cannot be represented as a value of
context type.

We do this already but we don't say it very clearly. Using a context type in the
explanations would make this much simpler to understand (and would also more
directly match a type-checker's implementation). For instance, the context type
in a variable declaration that also defines a type is that type. The context
type in a variable declaration that doesn't specify a type is the default type
of the initialization expression (i.e., the type of the expression, or the
respective default type for constants). The context type for x in a conversion
T(x) is T. And so forth.

With such a notion, we can simply say, that the ctype for x in a[x] is the
element type of a (the key type for maps, or "int" for arrays and slices).

I'll try to float a respective CL in the next few days. For now, I'll just
disable these tests.

https://codereview.appspot.com/7861045/diff/7001/test/shift1.go#newcode153
test/shift1.go:153: _ = float32(1.)       // ERROR "shift of type float32"
On 2013/03/15 23:49:39, remyoudompheng wrote:
> I don't think line 153 is an error.

of course not - copy-paste error. fixed.

[gri, 2013-03-18 21:12:18]

PTAL

(concrete error messages may need to be adjusted).

https://codereview.appspot.com/7861045/diff/7001/test/shift1.go
File test/shift1.go (right):

https://codereview.appspot.com/7861045/diff/7001/test/shift1.go#newcode143
test/shift1.go:143: _ = a[1.<<s]
On 2013/03/15 23:43:45, remyoudompheng wrote:
> rsc says that this is invalid on issue 4813, you seem to disagree.
> 
> I can't manage to say what  to think of it: 1.<<s should be typed as 1. in
> a[1.], but indexes can be untyped, what is the type of 1. in a[1.], can you
> confirm?

rsc thinks that we should leave this alone for now in the spec. Thus, a[1.] is
valid, but a[1.<<s] is not. I do think it is inconsistent, but at the same time
both compilers reject this and so let's leave it at that for Go 1.1, unless
everybody else changes their mind.

[remyoudompheng, 2013-03-18 22:43:03]

https://codereview.appspot.com/7861045/diff/22001/test/shift1.go
File test/shift1.go (right):

https://codereview.appspot.com/7861045/diff/22001/test/shift1.go#newcode147
test/shift1.go:147: _ = a[1.]
CL7613046 makes all test pass except this one, so no need to adjust messages.

Unmatched Errors:
shift1.go:147: non-integer slice index 1

https://codereview.appspot.com/7861045/diff/22001/test/shift1.go#newcode159
test/shift1.go:159: _ = float32(1.)       // ERROR "shift of type float32"
still an extra error here.

[gri, 2013-03-18 22:53:34]

PTAL

https://codereview.appspot.com/7861045/diff/22001/test/shift1.go
File test/shift1.go (right):

https://codereview.appspot.com/7861045/diff/22001/test/shift1.go#newcode147
test/shift1.go:147: _ = a[1.]
On 2013/03/18 22:43:03, remyoudompheng wrote:
> CL7613046 makes all test pass except this one, so no need to adjust messages.
> 
> Unmatched Errors:
> shift1.go:147: non-integer slice index 1

Added error for now. The spec is clear that this should be allowed. But that can
be another CL.

https://codereview.appspot.com/7861045/diff/22001/test/shift1.go#newcode159
test/shift1.go:159: _ = float32(1.)       // ERROR "shift of type float32"
On 2013/03/18 22:43:03, remyoudompheng wrote:
> still an extra error here.

Done.

[rsc, 2013-03-19 19:05:52]

LGTM

[remyoudompheng, 2013-03-21 23:47:07]

On 2013/03/19 19:05:52, rsc wrote:
> LGTM

gc is now ready for these tests.

[gri, 2013-03-21 23:57:19]

*** Submitted as https://code.google.com/p/go/source/detail?r=4dceacf2d92c ***

test: more systematic shift tests

To be submitted once gc agrees.

R=rsc, iant, remyoudompheng
CC=golang-dev
https://codereview.appspot.com/7861045