Issue 100480043: code review 100480043: cmd/gc: fix two select temporary bugs

Keyboard Shortcuts

	File
u :	up to issue
m :	publish + mail comments
M :	edit review message
j / k :	jump to file after / before current file
J / K :	jump to next file with a comment after / before current file
	Side-by-side diff
i :	toggle intra-line diffs
e :	expand all comments
c :	collapse all comments
s :	toggle showing all comments
n / p :	next / previous diff chunk or comment
N / P :	next / previous comment
<Up> / <Down> :	next / previous line
<Enter> :	respond to / edit current comment
d :	mark current comment as done

	Issue
u :	up to list of issues
m :	publish + mail comments
j / k :	jump to patch after / before current patch
o / <Enter> :	open current patch in side-by-side view
i :	open current patch in unified diff view

	Issue List
j / k :	jump to issue after / before current issue
o / <Enter> :	open current issue
# :	close issue

	Comment/message editing
<Ctrl> + s or <Ctrl> + Enter :	save comment
<Esc> :	cancel edit

Issue 100480043: code review 100480043: cmd/gc: fix two select temporary bugs (Closed)

Can't Edit
Can't Publish+Mail
Start Review

Created:
10 years ago by rsc

Modified:
10 years ago

Reviewers:
iant

CC:
golang-codereviews, iant

Visibility:
Public.

Description

cmd/gc: fix two select temporary bugs The introduction of temporaries in order.c was not quite right for two corner cases: 1) The rewrite that pushed new variables on the lhs of a receive into the body of the case was dropping the declaration of the variables. If the variables escape, the declaration is what allocates them. Caught by escape analysis sanity check. In fact the declarations should move into the body always, so that we only allocate if the corresponding case is selected. Do that. (This is an optimization that was already present in Go 1.2. The new order code just made it stop working.) Fixes issue 7997. 2) The optimization to turn a single-recv select into an ordinary receive assumed it could take the address of the destination; not so if the destination is _. Fixes issue 7998.

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+116 lines, -1 line)			Patch
M	src/cmd/gc/order.c	View	1 2	4 chunks	+36 lines, -0 lines	0 comments	Download
M	src/cmd/gc/walk.c	View	1	1 chunk	+4 lines, -1 line	0 comments	Download
A	test/fixedbugs/issue7997.go	View	1 2	1 chunk	+53 lines, -0 lines	0 comments	Download
A	test/fixedbugs/issue7998.go	View	1	1 chunk	+23 lines, -0 lines	0 comments	Download

Messages

Total messages: 3

Expand All Messages | Collapse All Messages

rsc

Hello golang-codereviews@googlegroups.com, I'd like you to review this change to https://code.google.com/p/go/

10 years ago (2014-05-15 03:10:19 UTC) #1

rsc

10 years ago (2014-05-15 23:16:25 UTC) #3

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

cmd/gc: fix two select temporary bugs

The introduction of temporaries in order.c was not
quite right for two corner cases:

1) The rewrite that pushed new variables on the lhs of
a receive into the body of the case was dropping the
declaration of the variables. If the variables escape,
the declaration is what allocates them.
Caught by escape analysis sanity check.
In fact the declarations should move into the body
always, so that we only allocate if the corresponding
case is selected. Do that. (This is an optimization that
was already present in Go 1.2. The new order code just
made it stop working.)

Fixes issue 7997.

2) The optimization to turn a single-recv select into
an ordinary receive assumed it could take the address
of the destination; not so if the destination is _.

Fixes issue 7998.

LGTM=iant
R=golang-codereviews, iant
CC=golang-codereviews
https://codereview.appspot.com/100480043

Expand All Messages | Collapse All Messages