code review 13367051: cmd/gc: allow append and complex builtins to accept 2-r...

cmd/gc: allow append and complex builtins to accept 2-result call expression as first argument.

Fixes issue 5793.

[cmang, 2013-09-11 18:45:32]

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

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

[rsc, 2013-09-11 18:51:15]

please run 'hg gofmt' to reformat the test case
(they are not strictly required to be formatted,
since some test cases depend on spacing or such,
but this one has no reason not to be)

[cmang, 2013-09-11 19:32:34]

On 2013/09/11 18:51:15, rsc wrote:
> please run 'hg gofmt' to reformat the test case
> (they are not strictly required to be formatted,
> since some test cases depend on spacing or such,
> but this one has no reason not to be)

I ran `hg gofmt` but it says 'no modified go files'. PTAL.

[rsc, 2013-09-11 21:15:27]

oh, right. and i explained why.

gofmt -w test/fixedbugs/issue5793.go then.
not a big deal.

i will review the CL in a few hours. i just saw the "No newline at end of
file" in the mail.

[rsc, 2013-09-12 16:15:02]

What's here looks good, and we could check it in as an intermediate step if you
like, but what about append(f()) where f returns []byte, byte, byte, byte? Is
that supposed to work? It looks like this code only handles the 2-result case.

https://codereview.appspot.com/13367051/diff/13001/src/cmd/gc/walk.c
File src/cmd/gc/walk.c (right):

https://codereview.appspot.com/13367051/diff/13001/src/cmd/gc/walk.c#newcode477
src/cmd/gc/walk.c:477: if (n->op == OCOMPLEX && n->left == N && n->right == N) {
s/if (/if(/
sorry

[cmang, 2013-09-12 16:29:06]

It actually does work for the case you mentioned because of the change to inl.c.
All the returned results from the multiple-valued call argument will be expanded
into append's argument list. Should I maybe modify the test case to show this to
avoid confusion?

https://codereview.appspot.com/13367051/diff/13001/src/cmd/gc/walk.c
File src/cmd/gc/walk.c (right):

https://codereview.appspot.com/13367051/diff/13001/src/cmd/gc/walk.c#newcode477
src/cmd/gc/walk.c:477: if (n->op == OCOMPLEX && n->left == N && n->right == N) {
On 2013/09/12 16:15:02, rsc wrote:
> s/if (/if(/
> sorry

Done.

[rsc, 2013-09-12 16:32:46]

Yes, definitely add a test.

I am not sure you can rely on the inlining trick. People can compile with
inlining disabled (go build -gcflags -l; that's minus ell).

[cmang, 2013-09-12 17:53:03]

On 2013/09/12 16:32:46, rsc wrote:
> Yes, definitely add a test.
> 
> I am not sure you can rely on the inlining trick. People can compile with
> inlining disabled (go build -gcflags -l; that's minus ell).

Just to update, I added an extra test to issue5793.go. Building the issue with
the inliner turned off is giving me strange results that I'm currently looking
in to. The data seems to be appended properly, but assignment causes a compiler
error due to a failed assignconv. I'll see if I can track that down within the
next couple of hours.

[cmang, 2013-09-13 22:33:46]

On 2013/09/12 17:53:03, cmang wrote:
> On 2013/09/12 16:32:46, rsc wrote:
> > Yes, definitely add a test.
> > 
> > I am not sure you can rely on the inlining trick. People can compile with
> > inlining disabled (go build -gcflags -l; that's minus ell).
> 
> Just to update, I added an extra test to issue5793.go. Building the issue with
> the inliner turned off is giving me strange results that I'm currently looking
> in to. The data seems to be appended properly, but assignment causes a
compiler
> error due to a failed assignconv. I'll see if I can track that down within the
> next couple of hours.

I've further modified walk.c and added a change to order.c to make this work
with inlining enabled or disabled. PTAL.

[rsc, 2013-09-23 17:34:41]

Let's leave this until after Go 1.2. Thanks.

[gobot, 2013-12-20 16:26:00]

Replacing golang-dev with golang-codereviews.

[dave_cheney.net, 2014-01-31 00:06:22]

On 2013/12/20 16:26:00, gobot wrote:
> Replacing golang-dev with golang-codereviews.

Chris, 

Could you please bring this CL up to date and hg mail it again.

[cmang, 2014-01-31 02:04:53]

On 2014/01/31 00:06:22, dfc wrote:
> On 2013/12/20 16:26:00, gobot wrote:
> > Replacing golang-dev with golang-codereviews.
> 
> Chris, 
> 
> Could you please bring this CL up to date and hg mail it again.

It should all be up-to-date now. There were no merging conflicts AFAICT. PTAL.

[rsc, 2014-03-05 19:12:15]

LGTM

https://codereview.appspot.com/13367051/diff/30001/src/cmd/gc/walk.c
File src/cmd/gc/walk.c (right):

https://codereview.appspot.com/13367051/diff/30001/src/cmd/gc/walk.c#newcode476
src/cmd/gc/walk.c:476: // Use results from call expression as arguments for
complex.
It is important that if you make the values accessible by new fields that you
zero out the old fields.
There are generic tree walks that just visit everything (left, right, list, ...)
and we do not want them to visit the args twice.

[rsc, 2014-03-05 19:16:25]

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

cmd/gc: allow append and complex builtins to accept 2-result call expression as
first argument.

Fixes issue 5793.

LGTM=rsc
R=rsc, adonovan, dave
CC=golang-codereviews
https://codereview.appspot.com/13367051

Committer: Russ Cox <rsc@golang.org>