code review 1957045: gc: permit omission of low slice bound (defaulting to 0)

gc: permit omission of low slice bound (defaulting to 0)
Fixes issue 382.

[bytbox, 2010-08-20 12:54:36]

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

I'd like you to review this change.

[gri1, 2010-08-20 16:37:42]

This is a (albeit backward-compatible) language change.

While there's nothing wrong in principle with this change, it does not solve
a real problem. For instance, a[0:i] is clearer than a[:i] at almost zero
cost (no pun intended). It may also be more easily confused with a[i:] when
glancing over it. In contrast, a[i:] solves a real problem, especially when
a is complicated.

Please hold off with this until we (the language designers) have a consensus
on this.
- gri


On Fri, Aug 20, 2010 at 5:54 AM, <bytbox@gmail.com> wrote:

> Reviewers: golang-dev_googlegroups.com,
>
> Message:
> Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com),
>
> I'd like you to review this change.
>
>
> Description:
> gc: permit omission of lo slice bound (defaulting to 0)
> Fixes issue 381.
>
> Please review this at http://codereview.appspot.com/1957045/
>
> Affected files:
>  M doc/go_spec.html
>  M src/cmd/gc/go.y
>  M src/cmd/gc/typecheck.c
>  M src/cmd/gc/walk.c
>
>
>

[bytbox, 2010-08-20 16:40:12]

Ok, sorry. Holding.

On 8/20/10, Robert Griesemer <gri@google.com> wrote:
> This is a (albeit backward-compatible) language change.
>
> While there's nothing wrong in principle with this change, it does not solve
> a real problem. For instance, a[0:i] is clearer than a[:i] at almost zero
> cost (no pun intended). It may also be more easily confused with a[i:] when
> glancing over it. In contrast, a[i:] solves a real problem, especially when
> a is complicated.
>
> Please hold off with this until we (the language designers) have a consensus
> on this.
> - gri
>
>
> On Fri, Aug 20, 2010 at 5:54 AM, <bytbox@gmail.com> wrote:
>
>> Reviewers: golang-dev_googlegroups.com,
>>
>> Message:
>> Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com),
>>
>> I'd like you to review this change.
>>
>>
>> Description:
>> gc: permit omission of lo slice bound (defaulting to 0)
>> Fixes issue 381.
>>
>> Please review this at http://codereview.appspot.com/1957045/
>>
>> Affected files:
>>  M doc/go_spec.html
>>  M src/cmd/gc/go.y
>>  M src/cmd/gc/typecheck.c
>>  M src/cmd/gc/walk.c
>>
>>
>>
>


-- 
Scott Lawrence

[gri, 2010-08-31 17:52:11]

The Go team has decided to accept this language change.

There should probably be some test cases. There's already a file
test/syntax/slice.go that needs to be adjusted. It can contain more test cases.

Russ, can you please look at the gc changes.

Russ, Rob, please review the language in the spec.

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

http://codereview.appspot.com/1957045/diff/2001/3001#newcode2186
doc/go_spec.html:2186: Slice          = "[" Expression ":" [ Expression ] "]" .
Slice          = "[" [ Expression ] ":" [ Expression ] "]" .

http://codereview.appspot.com/1957045/diff/2001/3001#newcode2456
doc/go_spec.html:2456: For convenience, either one of the <code>lo</code> and
<code>hi</code>
For convenience, either one or both of the index expression may be omitted. The
expressions <code>a[:hi]</code>, <code>a[lo:]</code>, and <code>a[:]</code> are
shorthand notations for <code>a[0 : hi]</code>, <code>a[lo : len(a)]</code>, and
<code>a[0 : len(a)]</code> respectively.

[cw, 2010-08-31 18:02:55]

please change the commit comment to refer to issue 382 (not 381)

[cw, 2010-08-31 18:55:07]

FWIW, using (a variant of) this, I get some problems.

  imgproc.go:391: internal compiler error: fault

[rsc1, 2010-08-31 19:26:31]

Is there something interesting on imgproc.go:391?


On Tue, Aug 31, 2010 at 14:55,  <cw@f00f.org> wrote:
> FWIW, using (a variant of) this, I get some problems.
>
>  imgproc.go:391: internal compiler error: fault
>
>
> http://codereview.appspot.com/1957045/
>

[cw, 2010-08-31 20:56:11]

On Tue, Aug 31, 2010 at 03:26:27PM -0400, Russ Cox wrote:

> Is there something interesting on imgproc.go:391?

this shows the problem, string related:

        _ = "123"[:1]

[cw, 2010-08-31 21:02:33]

more details

Program received signal SIGSEGV, Segmentation fault.
conv (n=0x0, t=0x66eae8) at walk.c:2200
2200            if(eqtype(n->type, t))
(gdb) where
#0  conv (n=0x0, t=0x66eae8) at walk.c:2200
#1  0x000000000042d0d8 in walkexpr (np=0x6acde8, init=0x7fffffffe338) at
walk.c:1225
#2  0x000000000042caf7 in walkexpr (np=0x7fffffffe330, init=0x7fffffffe338) at
walk.c:743
#3  0x000000000042b4c0 in walkstmt (np=0x6acee0) at walk.c:416
#4  0x000000000042f278 in walkstmtlist (l=0x6acee0) at walk.c:336
#5  0x000000000042f9f0 in walk (fn=<value optimized out>) at walk.c:87
#6  0x000000000040689b in compile (fn=0x6ac5b0) at
/home/cw/wk/go/go.hg/src/cmd/6g/ggen.c:53
#7  0x0000000000416759 in funccompile (n=0x6ac5b0, isclosure=0) at dcl.c:1191
#8  0x0000000000436639 in p9main (argc=<value optimized out>,
argv=0x7fffffffe6d0)
    at lex.c:258
#9  0x000000000043ae19 in main (argc=0, argv=0x66eae8) at main.c:35

[r, 2010-08-31 22:22:46]

Please fix the CL description to spell 'low' correctly.

[bytbox, 2010-08-31 22:33:06]

Hello golang-dev@googlegroups.com, gri, cw, rsc1, r (cc:
golang-dev@googlegroups.com),

Please take another look.

[bytbox, 2010-08-31 22:34:45]

That changes the documentation, but does not fix the string slice bug.


On 8/31/10, bytbox@gmail.com <bytbox@gmail.com> wrote:
> Hello golang-dev@googlegroups.com, gri, cw, rsc1, r (cc:
> golang-dev@googlegroups.com),
>
> Please take another look.
>
>
> http://codereview.appspot.com/1957045/
>


-- 
Scott Lawrence

[gri, 2010-09-01 22:22:39]

FYI.

If you separate the go_doc changes from the compiler changes, we can get them in
independently. The spec changes are fine (pending feedback from rsc, r), but
there appear to be compiler issues.
- gri

http://codereview.appspot.com/1957045/diff/28001/29001
File doc/go_spec.html (right):

http://codereview.appspot.com/1957045/diff/28001/29001#newcode2456
doc/go_spec.html:2456: For convenience, either one or both of the index
expression may be
s/expression/expressions/

(my bad, I think I had this wrong in the suggested wording)

[rsc1, 2010-09-02 01:11:59]

Yes, please move the compiler files into a new CL.

On Wed, Sep 1, 2010 at 18:22,  <gri@golang.org> wrote:
> FYI.
>
> If you separate the go_doc changes from the compiler changes, we can get
> them in independently. The spec changes are fine (pending feedback from
> rsc, r), but there appear to be compiler issues.
> - gri
>
>
> http://codereview.appspot.com/1957045/diff/28001/29001
> File doc/go_spec.html (right):
>
> http://codereview.appspot.com/1957045/diff/28001/29001#newcode2456
> doc/go_spec.html:2456: For convenience, either one or both of the index
> expression may be
> s/expression/expressions/
>
> (my bad, I think I had this wrong in the suggested wording)
>
> http://codereview.appspot.com/1957045/
>

[gri, 2010-09-07 17:50:57]

ping.

I'd like to move forward with this. If I don't hear from you within a day or
so, I will go ahead and make the suggested go_spec.html changes.
- gri

On Wed, Sep 1, 2010 at 3:22 PM, <gri@golang.org> wrote:

> FYI.
>
> If you separate the go_spec.html changes from the compiler changes, we can
> get
> them in independently. The spec changes are fine (pending feedback from
> rsc, r), but there appear to be compiler issues.
> - gri
>
>
> http://codereview.appspot.com/1957045/diff/28001/29001
>
> File doc/go_spec.html (right):
>
> http://codereview.appspot.com/1957045/diff/28001/29001#newcode2456
> doc/go_spec.html:2456: For convenience, either one or both of the index
> expression may be
> s/expression/expressions/
>
> (my bad, I think I had this wrong in the suggested wording)
>
>
> http://codereview.appspot.com/1957045/
>

[bytbox, 2010-09-07 20:39:11]

Ok, here's the go_spec CL: http://codereview.appspot.com/2163042

Fixing string slices.

[bytbox, 2010-09-07 21:15:07]

Hello golang-dev@googlegroups.com, gri, cw, rsc1, r (cc:
golang-dev@googlegroups.com),

Please take another look.

[bytbox, 2010-09-07 21:16:23]

On 09/07/2010 05:15 PM, bytbox@gmail.com wrote:
> Hello golang-dev@googlegroups.com, gri, cw, rsc1, r (cc:
> golang-dev@googlegroups.com),
> 
> Please take another look.
> 
> 
> http://codereview.appspot.com/1957045/
This (seems) to have fixed slices with strings (I have yet to test it
exhaustively); I haven't yet added test cases.

[cw, 2010-09-07 21:31:52]

On Tue, Sep 07, 2010 at 05:18:26PM -0400, Scott Lawrence wrote:

> This (seems) to have fixed slices with strings (I have yet to test it
> exhaustively); I haven't yet added test cases.

I'll retest in a few.

[cw, 2010-09-07 21:38:45]

LGTM (pending tests).  Certainly it's working here.

[rsc1, 2010-09-08 18:03:33]

LGTM

Will submit now so that I can work on related code.

[rsc1, 2010-09-08 18:03:35]

LGTM

Will submit now so that I can work on related code.

[bytbox, 2010-09-08 19:59:38]

On 2010/09/08 18:03:35, rsc1 wrote:
> LGTM
> 
> Will submit now so that I can work on related code.
Ok; I'll put tests in another CL.

[rsc, 2010-09-09 02:20:32]

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

gc: implement new slice spec
Fixes issue 382.

R=gri, cw, r, rsc
CC=golang-dev
http://codereview.appspot.com/1957045

Committer: Russ Cox <rsc@golang.org>