code review 5876044: cmd/gc: when expanding append inline, preserve arguments

cmd/gc: when expanding append inline, preserve arguments

Fixes issue 3369.

[iant, 2012-03-21 22:59:34]

Hello golang-dev@googlegroups.com,

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

[gri, 2012-03-21 23:16:09]

LGTM

[iant, 2012-03-21 23:54:56]

Luuk, since rsc is on vacation, can you take a quick look at this?

[r, 2012-03-22 02:18:08]

LGTM

[lvd1, 2012-03-22 10:14:42]

cheapexpr is supposed to be or quickly become a noop, the real fix should
be in order.c, let me quickly try that

On Thu, Mar 22, 2012 at 03:18, <r@golang.org> wrote:

> LGTM
>
>
http://codereview.appspot.com/**5876044/<http://codereview.appspot.com/5876044/>
>

[lvd1, 2012-03-22 12:46:07]

On Thu, Mar 22, 2012 at 11:14, Luuk van Dijk <lvd@golang.org> wrote:

> cheapexpr is supposed to be or quickly become a noop, the real fix should
> be in order.c, let me quickly try that
>
>
i was confused with safeexpr.  that arguments are evaluated before the
start of the function is enforced by the way calls (and inlining) work so
this is a special case.  here it should have been handled by the
walkexprlistsafe at line 2359, but safeexpr leaves n[i] alone if both n and
i are names or literals.  so i think the clearer fix would be to add just
below that a comment+loop like

// walkexprlistsafe/safeexpr will leave OINDEX (s[n]) alone if
// both s and n are name or literal, but those may index the
// slice we're modifying here.  Fix explicitly.
for (l = n->list; l; l = l->next)
l->n = cheapexpr(l->n, init);

...


> On Thu, Mar 22, 2012 at 03:18, <r@golang.org> wrote:
>
>> LGTM
>>
>>
http://codereview.appspot.com/**5876044/<http://codereview.appspot.com/5876044/>
>>
>
>

[lvd1, 2012-03-22 12:54:48]

On Thu, Mar 22, 2012 at 13:46, Luuk van Dijk <lvd@golang.org> wrote:

>
> On Thu, Mar 22, 2012 at 11:14, Luuk van Dijk <lvd@golang.org> wrote:
>
>> cheapexpr is supposed to be or quickly become a noop, the real fix should
>> be in order.c, let me quickly try that
>>
>>
> i was confused with safeexpr.  that arguments are evaluated before the
> start of the function is enforced by the way calls (and inlining) work so
> this is a special case.  here it should have been handled by the
> walkexprlistsafe at line 2359, but safeexpr leaves n[i] alone if both n and
> i are names or literals.  so i think the clearer fix would be to add just
> below that a comment+loop like
>
> // walkexprlistsafe/safeexpr will leave OINDEX (s[n]) alone if
> // both s and n are name or literal, but those may index the
>  // slice we're modifying here.  Fix explicitly.
> for (l = n->list; l; l = l->next)
>  l->n = cheapexpr(l->n, init);
>
>
LGTM either way.  but at least a comment explaining why it's neccesary
would be nice.


> ...
>
>
>> On Thu, Mar 22, 2012 at 03:18, <r@golang.org> wrote:
>>
>>> LGTM
>>>
>>>
http://codereview.appspot.com/**5876044/<http://codereview.appspot.com/5876044/>
>>>
>>
>>
>

[iant, 2012-03-22 16:44:35]

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

cmd/gc: when expanding append inline, preserve arguments

Fixes issue 3369.

R=golang-dev, gri, lvd, r
CC=golang-dev
http://codereview.appspot.com/5876044

[bradfitz, 2012-03-22 16:53:22]

s/along/alone/ typo

also, why spaces around one the = in one assignment but not the other
assignment?  I don't get this C style.

On Thu, Mar 22, 2012 at 9:44 AM, <iant@golang.org> wrote:

> *** Submitted as
>
http://code.google.com/p/go/**source/detail?r=75162220fd1e<http://code.google...
>
>
> cmd/gc: when expanding append inline, preserve arguments
>
> Fixes issue 3369.
>
> R=golang-dev, gri, lvd, r
> CC=golang-dev
> http://codereview.appspot.com/**5876044<http://codereview.appspot.com/5876044>
>
>
>
http://codereview.appspot.com/**5876044/<http://codereview.appspot.com/5876044/>
>

[iant2, 2012-03-22 17:58:05]

Brad Fitzpatrick <bradfitz@golang.org> writes:

> s/along/alone/ typo
>
> also, why spaces around one the = in one assignment but not the other
> assignment?  I don't get this C style.

Sigh.  I sent you a new CL.

Ian

[rsc, 2012-03-26 14:04:19]

LGTM

Ouch, thank you.