code review 4313062: gc: inline append when len<cap

gc: inline append when len<cap

fixes issue 1604

[rsc, 2011-03-31 15:21:51]

http://codereview.appspot.com/4313062/diff/2001/src/cmd/gc/subr.c
File src/cmd/gc/subr.c (left):

http://codereview.appspot.com/4313062/diff/2001/src/cmd/gc/subr.c#oldcode1135
src/cmd/gc/subr.c:1135: if(debug['r']) {
Would you mind leaving these in, just with if(0 && debug['r']) {
They are frequent trouble spots sure to come up again.

http://codereview.appspot.com/4313062/diff/2001/src/cmd/gc/walk.c
File src/cmd/gc/walk.c (right):

http://codereview.appspot.com/4313062/diff/2001/src/cmd/gc/walk.c#newcode2386
src/cmd/gc/walk.c:2386: Node *n, *i, *a, *b;
i is not a typical node name.  r is more common.

http://codereview.appspot.com/4313062/diff/2001/src/cmd/gc/walk.c#newcode2390
src/cmd/gc/walk.c:2390: // Check that it's an assignment of the form s =
append(s, elem...).
s/...//
(elem... has a different meaning)

http://codereview.appspot.com/4313062/diff/2001/src/cmd/gc/walk.c#newcode2403
src/cmd/gc/walk.c:2403: i->nbody = list1(nod(OAS, n->left, (b=nod(OSLICE,
n->left, nod(OKEY, N, nod(OADD, a, nodintconst(1)))))));
please hoist the assignments into their own statements.

http://codereview.appspot.com/4313062/diff/2001/src/cmd/gc/walk.c#newcode2411
src/cmd/gc/walk.c:2411: i->nelse->n->dodata = n->dodata;
You don't need to set this.
The dodata flag tracks whether this particular
assignment should be compiled into a data
statement (because it is known at compile time
and is a global variable).  This is surely not.

[lvd, 2011-03-31 18:06:31]

all done

http://codereview.appspot.com/4313062/diff/2001/src/cmd/gc/subr.c
File src/cmd/gc/subr.c (left):

http://codereview.appspot.com/4313062/diff/2001/src/cmd/gc/subr.c#oldcode1135
src/cmd/gc/subr.c:1135: if(debug['r']) {
On 2011/03/31 15:21:51, rsc wrote:
> Would you mind leaving these in, just with if(0 && debug['r']) {
> They are frequent trouble spots sure to come up again.

Done.

http://codereview.appspot.com/4313062/diff/2001/src/cmd/gc/walk.c
File src/cmd/gc/walk.c (right):

http://codereview.appspot.com/4313062/diff/2001/src/cmd/gc/walk.c#newcode2386
src/cmd/gc/walk.c:2386: Node *n, *i, *a, *b;
On 2011/03/31 15:21:51, rsc wrote:
> i is not a typical node name.  r is more common.

Done.  it was 'i' for the 'if' node.

http://codereview.appspot.com/4313062/diff/2001/src/cmd/gc/walk.c#newcode2390
src/cmd/gc/walk.c:2390: // Check that it's an assignment of the form s =
append(s, elem...).
On 2011/03/31 15:21:51, rsc wrote:
> s/...//
> (elem... has a different meaning)

Done.

http://codereview.appspot.com/4313062/diff/2001/src/cmd/gc/walk.c#newcode2403
src/cmd/gc/walk.c:2403: i->nbody = list1(nod(OAS, n->left, (b=nod(OSLICE,
n->left, nod(OKEY, N, nod(OADD, a, nodintconst(1)))))));
On 2011/03/31 15:21:51, rsc wrote:
> please hoist the assignments into their own statements.

i find it less clear that it says what it says then, but as you wish.

http://codereview.appspot.com/4313062/diff/2001/src/cmd/gc/walk.c#newcode2411
src/cmd/gc/walk.c:2411: i->nelse->n->dodata = n->dodata;
oOn 2011/03/31 15:21:51, rsc wrote:
> You don't need to set this.
> The dodata flag tracks whether this particular
> assignment should be compiled into a data
> statement (because it is known at compile time
> and is a global variable).  This is surely not.

ok.  i had no idea what it was for, just cargocultcopied from the original.

[lvd, 2011-04-13 19:21:44]

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

I'd like you to review this change to
https://go.googlecode.com/hg/

[bradfitz, 2011-04-13 19:27:26]

Is this for http://code.google.com/p/go/issues/detail?id=1604 ?

Might want to update the commit message if so.

On Wed, Apr 13, 2011 at 12:21 PM, <lvd@google.com> wrote:

> Hello rsc (cc: golang-dev@googlegroups.com),
>
> I'd like you to review this change to
> https://go.googlecode.com/hg/
>
>
> http://codereview.appspot.com/4313062/
>

[rsc, 2011-04-13 20:03:17]

http://codereview.appspot.com/4313062/diff/10011/src/cmd/6g/gsubr.c
File src/cmd/6g/gsubr.c (right):

http://codereview.appspot.com/4313062/diff/10011/src/cmd/6g/gsubr.c#newcode914
src/cmd/6g/gsubr.c:914: if(0&& as == ALEAQ && f->op == OREGISTER) {
s/0/0 /

http://codereview.appspot.com/4313062/diff/10011/src/cmd/6g/gsubr.c#newcode915
src/cmd/6g/gsubr.c:915: dump("ALEAQ on bad from:", f);
tabs

http://codereview.appspot.com/4313062/diff/10011/src/cmd/6g/gsubr.c#newcode978
src/cmd/6g/gsubr.c:978: // reference with large offset.	 instead, emit explicit
spurious tab

http://codereview.appspot.com/4313062/diff/10011/src/cmd/6g/gsubr.c#newcode1830
src/cmd/6g/gsubr.c:1830: case AORB:  case AORW:	case AORL:  case AORQ:
spurious tab

http://codereview.appspot.com/4313062/diff/10011/src/cmd/6g/list.c
File src/cmd/6g/list.c (right):

http://codereview.appspot.com/4313062/diff/10011/src/cmd/6g/list.c#newcode123
src/cmd/6g/list.c:123: snprint(str, sizeof(str), "%S+%lld(SP)[%d]", a->sym,
a->offset, a->width);
please drop
it was great for debugging but will be confusing in the normal output

http://codereview.appspot.com/4313062/diff/10011/src/cmd/gc/walk.c
File src/cmd/gc/walk.c (right):

http://codereview.appspot.com/4313062/diff/10011/src/cmd/gc/walk.c#newcode2374
src/cmd/gc/walk.c:2374: //   m := n + argc
This addition can overflow.  Worse, all the bounds checking
is turned off afterward based on the check that uses m, so it
won't be caught.

I think the code pattern here should be

const argc = len(args)-1
if cap(s) - len(s) < argc {
    s = runtime.growslice(s, argc)
}
n := len(s)
s = s[:n+cap(s)]
s[n] = a
s[n+1] = b
...

Keeping the temporaries across the call to growslice
ensures that they will take up stack space.  The code above
avoids that.  It also avoids the overflow problem by 
comparing against cap-len and by passing argc to growslice.

http://codereview.appspot.com/4313062/diff/10011/src/cmd/gc/walk.c#newcode2376
src/cmd/gc/walk.c:2376: //        growslice(s, m)
s =

http://codereview.appspot.com/4313062/diff/10011/src/cmd/gc/walk.c#newcode2397
src/cmd/gc/walk.c:2397: ns = cheapexpr(n->left, init);
add
walkexprlistsafe(n->list, init);
so that the arguments in n->list will no longer contain side effects.

as written i believe this has not yet happened,
and without it, this program will fail:

package main

var x = make([]int, 0, 10)

func f() int {
    x[:1][0] = 3
    return 2
}

func main() {
    x = append(x, 1, f())
    if x[0] != 1 || x[1] != 2 {
        println("append failed: ", x[0], x[1])
    }
}

http://codereview.appspot.com/4313062/diff/10011/src/cmd/gc/walk.c#newcode2401
src/cmd/gc/walk.c:2401: yyerror("append call needs at least 1 appendum");
appendum?

I don't think this is an error anyway.  
We don't reject x += 0.
The spec certainly doesn't say it is an error.
Should handle (generate no code).

http://codereview.appspot.com/4313062/diff/10011/src/cmd/gc/walk.c#newcode2435
src/cmd/gc/walk.c:2435: a = a->next;                                    // first
one is s, skip
for(a=n->right->list->next; a != nil; a=a->next) {
...
}

http://codereview.appspot.com/4313062/diff/10011/src/pkg/runtime/slice.c
File src/pkg/runtime/slice.c (right):

http://codereview.appspot.com/4313062/diff/10011/src/pkg/runtime/slice.c#newc...
src/pkg/runtime/slice.c:135: array = x.array;     // ret may be an alias of x,
so makeslice1 clobbers it
no it can't.  x is a local variable

[lvd, 2011-04-13 20:19:18]

preliminary results of the benchmark in issue 1604:

in the report:
bm.BenchmarkAppend	 1000000	      2763 ns/op
bm.BenchmarkSpecialCase	20000000	       110 ns/op

in the new version on my macbook:
runtime_test.BenchmarkAppend    50000000                53 ns/op
runtime_test.BenchmarkSpecialCase       50000000                71 ns/op

i'll address the comments tomorrow

On Wed, Apr 13, 2011 at 22:03,  <rsc@golang.org> wrote:
>
> http://codereview.appspot.com/4313062/diff/10011/src/cmd/6g/gsubr.c
> File src/cmd/6g/gsubr.c (right):
>
> http://codereview.appspot.com/4313062/diff/10011/src/cmd/6g/gsubr.c#newcode914
> src/cmd/6g/gsubr.c:914: if(0&& as == ALEAQ && f->op == OREGISTER) {
> s/0/0 /
>
> http://codereview.appspot.com/4313062/diff/10011/src/cmd/6g/gsubr.c#newcode915
> src/cmd/6g/gsubr.c:915: dump("ALEAQ on bad from:", f);
> tabs
>
> http://codereview.appspot.com/4313062/diff/10011/src/cmd/6g/gsubr.c#newcode978
> src/cmd/6g/gsubr.c:978: // reference with large offset.  instead, emit
> explicit
> spurious tab
>
>
http://codereview.appspot.com/4313062/diff/10011/src/cmd/6g/gsubr.c#newcode1830
> src/cmd/6g/gsubr.c:1830: case AORB:  case AORW: case AORL:  case AORQ:
> spurious tab
>
> http://codereview.appspot.com/4313062/diff/10011/src/cmd/6g/list.c
> File src/cmd/6g/list.c (right):
>
> http://codereview.appspot.com/4313062/diff/10011/src/cmd/6g/list.c#newcode123
> src/cmd/6g/list.c:123: snprint(str, sizeof(str), "%S+%lld(SP)[%d]",
> a->sym, a->offset, a->width);
> please drop
> it was great for debugging but will be confusing in the normal output
>
> http://codereview.appspot.com/4313062/diff/10011/src/cmd/gc/walk.c
> File src/cmd/gc/walk.c (right):
>
> http://codereview.appspot.com/4313062/diff/10011/src/cmd/gc/walk.c#newcode2374
> src/cmd/gc/walk.c:2374: //   m := n + argc
> This addition can overflow.  Worse, all the bounds checking
> is turned off afterward based on the check that uses m, so it
> won't be caught.
>
> I think the code pattern here should be
>
> const argc = len(args)-1
> if cap(s) - len(s) < argc {
>    s = runtime.growslice(s, argc)
> }
> n := len(s)
> s = s[:n+cap(s)]
> s[n] = a
> s[n+1] = b
> ...
>
> Keeping the temporaries across the call to growslice
> ensures that they will take up stack space.  The code above
> avoids that.  It also avoids the overflow problem by
> comparing against cap-len and by passing argc to growslice.
>
> http://codereview.appspot.com/4313062/diff/10011/src/cmd/gc/walk.c#newcode2376
> src/cmd/gc/walk.c:2376: //        growslice(s, m)
> s =
>
> http://codereview.appspot.com/4313062/diff/10011/src/cmd/gc/walk.c#newcode2397
> src/cmd/gc/walk.c:2397: ns = cheapexpr(n->left, init);
> add
> walkexprlistsafe(n->list, init);
> so that the arguments in n->list will no longer contain side effects.
>
> as written i believe this has not yet happened,
> and without it, this program will fail:
>
> package main
>
> var x = make([]int, 0, 10)
>
> func f() int {
>    x[:1][0] = 3
>    return 2
> }
>
> func main() {
>    x = append(x, 1, f())
>    if x[0] != 1 || x[1] != 2 {
>        println("append failed: ", x[0], x[1])
>    }
> }
>
> http://codereview.appspot.com/4313062/diff/10011/src/cmd/gc/walk.c#newcode2401
> src/cmd/gc/walk.c:2401: yyerror("append call needs at least 1
> appendum");
> appendum?
>
> I don't think this is an error anyway.
> We don't reject x += 0.
> The spec certainly doesn't say it is an error.
> Should handle (generate no code).
>
> http://codereview.appspot.com/4313062/diff/10011/src/cmd/gc/walk.c#newcode2435
> src/cmd/gc/walk.c:2435: a = a->next;
> // first one is s, skip
> for(a=n->right->list->next; a != nil; a=a->next) {
> ...
> }
>
> http://codereview.appspot.com/4313062/diff/10011/src/pkg/runtime/slice.c
> File src/pkg/runtime/slice.c (right):
>
>
http://codereview.appspot.com/4313062/diff/10011/src/pkg/runtime/slice.c#newc...
> src/pkg/runtime/slice.c:135: array = x.array;     // ret may be an alias
> of x, so makeslice1 clobbers it
> no it can't.  x is a local variable
>
> http://codereview.appspot.com/4313062/
>

[lvd, 2011-04-14 09:34:07]

all done, the tree compiles and timngs of the benchmark still same.

http://codereview.appspot.com/4313062/diff/10011/src/cmd/6g/gsubr.c
File src/cmd/6g/gsubr.c (right):

http://codereview.appspot.com/4313062/diff/10011/src/cmd/6g/gsubr.c#newcode914
src/cmd/6g/gsubr.c:914: if(0&& as == ALEAQ && f->op == OREGISTER) {
On 2011/04/13 20:03:21, rsc wrote:
> s/0/0 /

Done.

http://codereview.appspot.com/4313062/diff/10011/src/cmd/6g/gsubr.c#newcode915
src/cmd/6g/gsubr.c:915: dump("ALEAQ on bad from:", f);
On 2011/04/13 20:03:21, rsc wrote:
> tabs

Done.

http://codereview.appspot.com/4313062/diff/10011/src/cmd/6g/gsubr.c#newcode978
src/cmd/6g/gsubr.c:978: // reference with large offset.	 instead, emit explicit
On 2011/04/13 20:03:21, rsc wrote:
> spurious tab

Done.

http://codereview.appspot.com/4313062/diff/10011/src/cmd/6g/gsubr.c#newcode1830
src/cmd/6g/gsubr.c:1830: case AORB:  case AORW:	case AORL:  case AORQ:
On 2011/04/13 20:03:21, rsc wrote:
> spurious tab

Done.

http://codereview.appspot.com/4313062/diff/10011/src/cmd/6g/list.c
File src/cmd/6g/list.c (right):

http://codereview.appspot.com/4313062/diff/10011/src/cmd/6g/list.c#newcode123
src/cmd/6g/list.c:123: snprint(str, sizeof(str), "%S+%lld(SP)[%d]", a->sym,
a->offset, a->width);
On 2011/04/13 20:03:21, rsc wrote:
> please drop
> it was great for debugging but will be confusing in the normal output

Done.

http://codereview.appspot.com/4313062/diff/10011/src/cmd/gc/walk.c
File src/cmd/gc/walk.c (right):

http://codereview.appspot.com/4313062/diff/10011/src/cmd/gc/walk.c#newcode2374
src/cmd/gc/walk.c:2374: //   m := n + argc
On 2011/04/13 20:03:21, rsc wrote:
> This addition can overflow.  Worse, all the bounds checking
> is turned off afterward based on the check that uses m, so it
> won't be caught.
> 
> I think the code pattern here should be
> 
> const argc = len(args)-1
> if cap(s) - len(s) < argc {
>     s = runtime.growslice(s, argc)
> }
> n := len(s)
> s = s[:n+cap(s)]

cap(s) is now the new one, but i can use n + argc 

> s[n] = a
> s[n+1] = b
> ...
> 
> Keeping the temporaries across the call to growslice
> ensures that they will take up stack space.  The code above
> avoids that.  It also avoids the overflow problem by 
> comparing against cap-len and by passing argc to growslice.

http://codereview.appspot.com/4313062/diff/10011/src/cmd/gc/walk.c#newcode2397
src/cmd/gc/walk.c:2397: ns = cheapexpr(n->left, init);
On 2011/04/13 20:03:21, rsc wrote:
> add
> walkexprlistsafe(n->list, init);
> so that the arguments in n->list will no longer contain side effects.
> 
> as written i believe this has not yet happened,
> and without it, this program will fail:
> 
> package main
> 
> var x = make([]int, 0, 10)
> 
> func f() int {
>     x[:1][0] = 3
>     return 2
> }
> 
> func main() {
>     x = append(x, 1, f())
>     if x[0] != 1 || x[1] != 2 {
>         println("append failed: ", x[0], x[1])
>     }
> }

Done.

http://codereview.appspot.com/4313062/diff/10011/src/cmd/gc/walk.c#newcode2401
src/cmd/gc/walk.c:2401: yyerror("append call needs at least 1 appendum");
On 2011/04/13 20:03:21, rsc wrote:
> appendum?
> 
> I don't think this is an error anyway.  
> We don't reject x += 0.
> The spec certainly doesn't say it is an error.
> Should handle (generate no code).
> 

Done, that is, replaced with s = s, assuming that will be optimized away without
potentially triggering 's not used'.  there doesn't seem to be an ONOP.

http://codereview.appspot.com/4313062/diff/10011/src/cmd/gc/walk.c#newcode2435
src/cmd/gc/walk.c:2435: a = a->next;                                    // first
one is s, skip
On 2011/04/13 20:03:21, rsc wrote:
> for(a=n->right->list->next; a != nil; a=a->next) {
> ...
> }

Done.

http://codereview.appspot.com/4313062/diff/10011/src/pkg/runtime/slice.c
File src/pkg/runtime/slice.c (right):

http://codereview.appspot.com/4313062/diff/10011/src/pkg/runtime/slice.c#newc...
src/pkg/runtime/slice.c:135: array = x.array;     // ret may be an alias of x,
so makeslice1 clobbers it
On 2011/04/13 20:03:21, rsc wrote:
> no it can't.  x is a local variable
doh.

[rsc, 2011-04-14 15:12:08]

Code looks good.  The next step is to make the
same changes to 5g and 8g.  It helps to do this
in the same CL, since you often find when making
the changes to the others that you didn't get something
quite right in the first.  

Testing 8g is trivial: export GOARCH=386.

To test 5g, find an Ubuntu Linux box and run

    sudo apt-get install qemu-kvm-extras-static

and then

    cd $GOROOT/src
    GOARCH=arm DISABLE_NET_TESTS=1 ./all-qemu.bash

http://codereview.appspot.com/4313062/diff/23001/src/cmd/gc/walk.c
File src/cmd/gc/walk.c (right):

http://codereview.appspot.com/4313062/diff/23001/src/cmd/gc/walk.c#newcode2398
src/cmd/gc/walk.c:2398: n->right = n->left;  // s = append(s) => s = s (ONOP?)
n->op = OEMPTY;

http://codereview.appspot.com/4313062/diff/23001/src/cmd/gc/walk.c#newcode2402
src/cmd/gc/walk.c:2402: walkexprlistsafe(n->list, init);
move this one line down.
you have to get the side effects from n->left onto init
(via cheapexpr) before the side effects from n->list
(via walkexprlistsafe), because the language spec says
that the side effects (that is, function calls) are
executed in the order on the line.

right now n->left is always simple (just a name)
so it doesn't really matter, but it's worth getting
right now in case the code expands its scope later.

for the same reason, move both lines up above the
argc check, so that you have:

ns = cheapexpr...
walkexprlistsafe...

argc = ...
...

http://codereview.appspot.com/4313062/diff/23001/src/pkg/runtime/append_test.go
File src/pkg/runtime/append_test.go (right):

http://codereview.appspot.com/4313062/diff/23001/src/pkg/runtime/append_test....
src/pkg/runtime/append_test.go:6: 
since you've got a test file anyway, please add the test case
i sent in my last code review

http://codereview.appspot.com/4313062/diff/23001/src/pkg/runtime/slice.c
File src/pkg/runtime/slice.c (right):

http://codereview.appspot.com/4313062/diff/23001/src/pkg/runtime/slice.c#newc...
src/pkg/runtime/slice.c:99: if (n < 1) { // if we're gonna check, but only to
panic about it we may as well ignore it
I still have no idea what this comment says.
Just delete the whole statement.

[lvd, 2011-04-19 00:23:11]

i started on the 8g stuff but took it out as it's unlikely that i can get all
the bugs stamped out before i go offline for a few weeks.

will test all again and submit as discussed.

http://codereview.appspot.com/4313062/diff/23001/src/cmd/gc/walk.c
File src/cmd/gc/walk.c (right):

http://codereview.appspot.com/4313062/diff/23001/src/cmd/gc/walk.c#newcode2398
src/cmd/gc/walk.c:2398: n->right = n->left;  // s = append(s) => s = s (ONOP?)
On 2011/04/14 15:12:08, rsc wrote:
> n->op = OEMPTY;

Done.

http://codereview.appspot.com/4313062/diff/23001/src/cmd/gc/walk.c#newcode2402
src/cmd/gc/walk.c:2402: walkexprlistsafe(n->list, init);
On 2011/04/14 15:12:08, rsc wrote:
> move this one line down.
> you have to get the side effects from n->left onto init
> (via cheapexpr) before the side effects from n->list
> (via walkexprlistsafe), because the language spec says
> that the side effects (that is, function calls) are
> executed in the order on the line.
> 
> right now n->left is always simple (just a name)
> so it doesn't really matter, but it's worth getting
> right now in case the code expands its scope later.
> 
> for the same reason, move both lines up above the
> argc check, so that you have:
> 
> ns = cheapexpr...
> walkexprlistsafe...
> 
> argc = ...
> ...

Done.

http://codereview.appspot.com/4313062/diff/23001/src/pkg/runtime/append_test.go
File src/pkg/runtime/append_test.go (right):

http://codereview.appspot.com/4313062/diff/23001/src/pkg/runtime/append_test....
src/pkg/runtime/append_test.go:6: 
On 2011/04/14 15:12:08, rsc wrote:
> since you've got a test file anyway, please add the test case
> i sent in my last code review

Done.

http://codereview.appspot.com/4313062/diff/23001/src/pkg/runtime/slice.c
File src/pkg/runtime/slice.c (right):

http://codereview.appspot.com/4313062/diff/23001/src/pkg/runtime/slice.c#newc...
src/pkg/runtime/slice.c:99: if (n < 1) { // if we're gonna check, but only to
panic about it we may as well ignore it
On 2011/04/14 15:12:08, rsc wrote:
> I still have no idea what this comment says.
> Just delete the whole statement.

first i thought i should check newcap >= cap, in the if... panic below, but then
i thought if i'm going to pay the price of chekcing it, i may as well do it to
handle it gracefully.  then again, growslice should never be called with a
negative argument

done as discussed

[lvd, 2011-05-11 13:16:29]

tested on linux, all tests pass. cleaned up some commentedout dump's in walk.c
should be ready.

[lvd, 2011-05-11 13:18:57]

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

Please take another look.

[rsc, 2011-05-11 14:16:58]

LGTM

http://codereview.appspot.com/4313062/diff/40014/src/cmd/6g/ggen.c
File src/cmd/6g/ggen.c (right):

http://codereview.appspot.com/4313062/diff/40014/src/cmd/6g/ggen.c#newcode199
src/cmd/6g/ggen.c:199: 
trailing tab

http://codereview.appspot.com/4313062/diff/40014/src/cmd/6g/gsubr.c
File src/cmd/6g/gsubr.c (right):

http://codereview.appspot.com/4313062/diff/40014/src/cmd/6g/gsubr.c#newcode51
src/cmd/6g/gsubr.c:51: * linked into program.	 pc is next instruction.
s/tab/space/

http://codereview.appspot.com/4313062/diff/40014/src/cmd/6g/gsubr.c#newcode914
src/cmd/6g/gsubr.c:914: if(0 && as == ALEAQ && f->op == OREGISTER)
delete?

http://codereview.appspot.com/4313062/diff/40014/src/cmd/6g/gsubr.c#newcode959
src/cmd/6g/gsubr.c:959: print("bad width: %P (%d, %d)\n", p, af.width,
at.width);
s/print/fatal/

http://codereview.appspot.com/4313062/diff/40014/src/cmd/6g/gsubr.c#newcode960
src/cmd/6g/gsubr.c:960: *(int*)0=0;
delete

http://codereview.appspot.com/4313062/diff/40014/src/cmd/6g/reg.c
File src/cmd/6g/reg.c (right):

http://codereview.appspot.com/4313062/diff/40014/src/cmd/6g/reg.c#newcode876
src/cmd/6g/reg.c:876: //				print("disable overlap %s %d %d %d %d, %E != %E\n",
s->name, v->offset, v->width, o, w, v->etype, et);
tabs if they're not there already

http://codereview.appspot.com/4313062/diff/40014/src/cmd/gc/walk.c
File src/cmd/gc/walk.c (right):

http://codereview.appspot.com/4313062/diff/40014/src/cmd/gc/walk.c#newcode814
src/cmd/gc/walk.c:814: if (/*debug['r'] &&*/ oasappend(&n, init))
drop debug r

http://codereview.appspot.com/4313062/diff/40014/src/pkg/runtime/append_test.go
File src/pkg/runtime/append_test.go (right):

http://codereview.appspot.com/4313062/diff/40014/src/pkg/runtime/append_test....
src/pkg/runtime/append_test.go:1: package runtime_test
copyright

[rsc, 2011-05-11 14:17:37]

change CL decription

issue 1604 ->

Fixes issue 1604.

(that's a command for the issue tracker)

[lvd, 2011-05-11 14:24:33]

all done. submitting

http://codereview.appspot.com/4313062/diff/40014/src/cmd/6g/ggen.c
File src/cmd/6g/ggen.c (right):

http://codereview.appspot.com/4313062/diff/40014/src/cmd/6g/ggen.c#newcode199
src/cmd/6g/ggen.c:199: 
On 2011/05/11 14:16:58, rsc wrote:
> trailing tab

Done.

http://codereview.appspot.com/4313062/diff/40014/src/cmd/6g/gsubr.c
File src/cmd/6g/gsubr.c (right):

http://codereview.appspot.com/4313062/diff/40014/src/cmd/6g/gsubr.c#newcode51
src/cmd/6g/gsubr.c:51: * linked into program.	 pc is next instruction.
On 2011/05/11 14:16:58, rsc wrote:
> s/tab/space/

Done.

http://codereview.appspot.com/4313062/diff/40014/src/cmd/6g/gsubr.c#newcode914
src/cmd/6g/gsubr.c:914: if(0 && as == ALEAQ && f->op == OREGISTER)
On 2011/05/11 14:16:58, rsc wrote:
> delete?

Done.

http://codereview.appspot.com/4313062/diff/40014/src/cmd/6g/gsubr.c#newcode959
src/cmd/6g/gsubr.c:959: print("bad width: %P (%d, %d)\n", p, af.width,
at.width);
On 2011/05/11 14:16:58, rsc wrote:
> s/print/fatal/

Done.

http://codereview.appspot.com/4313062/diff/40014/src/cmd/6g/gsubr.c#newcode960
src/cmd/6g/gsubr.c:960: *(int*)0=0;
On 2011/05/11 14:16:58, rsc wrote:
> delete

Done.

http://codereview.appspot.com/4313062/diff/40014/src/cmd/6g/reg.c
File src/cmd/6g/reg.c (right):

http://codereview.appspot.com/4313062/diff/40014/src/cmd/6g/reg.c#newcode876
src/cmd/6g/reg.c:876: //				print("disable overlap %s %d %d %d %d, %E != %E\n",
s->name, v->offset, v->width, o, w, v->etype, et);
On 2011/05/11 14:16:58, rsc wrote:
> tabs if they're not there already
> 

Done.

http://codereview.appspot.com/4313062/diff/40014/src/cmd/gc/walk.c
File src/cmd/gc/walk.c (right):

http://codereview.appspot.com/4313062/diff/40014/src/cmd/gc/walk.c#newcode814
src/cmd/gc/walk.c:814: if (/*debug['r'] &&*/ oasappend(&n, init))
On 2011/05/11 14:16:58, rsc wrote:
> drop debug r

Done.

http://codereview.appspot.com/4313062/diff/40014/src/pkg/runtime/append_test.go
File src/pkg/runtime/append_test.go (right):

http://codereview.appspot.com/4313062/diff/40014/src/pkg/runtime/append_test....
src/pkg/runtime/append_test.go:1: package runtime_test
On 2011/05/11 14:16:58, rsc wrote:
> copyright

Done.

[lvd, 2011-05-11 14:35:19]

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

gc: inline append when len<cap

issue 1604

R=rsc, bradfitz
CC=golang-dev
http://codereview.appspot.com/4313062

[peterGo, 2011-05-11 14:58:51]

On 2011/05/11 14:24:33, lvd wrote:
> all done. submitting

You didn't do this:

On 2011/05/11 14:17:37, rsc wrote:
> change CL decription
> 
> issue 1604 ->
> 
> Fixes issue 1604.
> 
> (that's a command for the issue tracker)