Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(538)

Issue 6498078: code review 6498078: reflect: add Select (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
10 years ago by rsc
Modified:
10 years ago
Reviewers:
CC:
r, iant, rog, bradfitz, golang-dev
Visibility:
Public.

Description

reflect: add Select

Patch Set 1 #

Patch Set 2 : diff -r 33a62d86ca2f https://code.google.com/p/go/ #

Patch Set 3 : diff -r df382f6986cf https://code.google.com/p/go/ #

Patch Set 4 : diff -r df382f6986cf https://code.google.com/p/go/ #

Total comments: 21

Patch Set 5 : diff -r e3fce06291ec https://code.google.com/p/go/ #

Patch Set 6 : diff -r e3fce06291ec https://code.google.com/p/go/ #

Total comments: 5

Patch Set 7 : diff -r cb6940e19fe2 https://go.googlecode.com/hg/ #

Patch Set 8 : diff -r cb6940e19fe2 https://go.googlecode.com/hg/ #

Patch Set 9 : diff -r cb6940e19fe2 https://go.googlecode.com/hg/ #

Total comments: 4

Patch Set 10 : diff -r 6cfab3a0935e https://go.googlecode.com/hg/ #

Patch Set 11 : diff -r 6cfab3a0935e https://go.googlecode.com/hg/ #

Unified diffs Side-by-side diffs Delta from patch set Stats (+654 lines, -8 lines) Patch
M src/pkg/reflect/all_test.go View 1 2 3 4 5 6 7 8 9 10 3 chunks +424 lines, -0 lines 0 comments Download
M src/pkg/reflect/type.go View 1 2 3 4 5 6 2 chunks +6 lines, -5 lines 0 comments Download
M src/pkg/reflect/value.go View 1 2 3 4 5 6 7 8 9 10 1 chunk +134 lines, -0 lines 0 comments Download
M src/pkg/runtime/chan.c View 1 2 3 4 5 6 7 8 9 2 chunks +90 lines, -3 lines 0 comments Download

Messages

Total messages: 20
rsc
Hello golang-dev@googlegroups.com, I'd like you to review this change to https://code.google.com/p/go/
10 years ago (2012-09-05 14:54:13 UTC) #1
r
http://codereview.appspot.com/6498078/diff/2002/src/pkg/reflect/all_test.go File src/pkg/reflect/all_test.go (right): http://codereview.appspot.com/6498078/diff/2002/src/pkg/reflect/all_test.go#newcode1062 src/pkg/reflect/all_test.go:1062: // 5 recv from closed channel probably want a ...
10 years ago (2012-09-05 15:07:04 UTC) #2
iant
LGTM https://codereview.appspot.com/6498078/diff/2002/src/pkg/reflect/all_test.go File src/pkg/reflect/all_test.go (right): https://codereview.appspot.com/6498078/diff/2002/src/pkg/reflect/all_test.go#newcode1062 src/pkg/reflect/all_test.go:1062: // 5 recv from closed channel On 2012/09/05 ...
10 years ago (2012-09-05 15:53:10 UTC) #3
r
It's so easy to implement nil channels I think it should be done. That means ...
10 years ago (2012-09-05 15:56:05 UTC) #4
rsc
On Wed, Sep 5, 2012 at 11:56 AM, Rob Pike <r@golang.org> wrote: > It's so ...
10 years ago (2012-09-05 21:41:59 UTC) #5
r
I'm surprised to hear all this pushback about taking a panic *out* of a library ...
10 years ago (2012-09-05 22:47:47 UTC) #6
rog
It's great to see this going in. I have a reservation about the API, though. ...
10 years ago (2012-09-10 10:11:40 UTC) #7
rsc
Yes, I will introduce a separate enumeration for the case kinds.
10 years ago (2012-09-10 16:37:55 UTC) #8
rsc
PTAL. You can ignore the test - I have an idea for cleaning it up. ...
10 years ago (2012-09-13 21:26:55 UTC) #9
iant
LGTM https://codereview.appspot.com/6498078/diff/8007/src/pkg/runtime/type.h File src/pkg/runtime/type.h (right): https://codereview.appspot.com/6498078/diff/8007/src/pkg/runtime/type.h#newcode92 src/pkg/runtime/type.h:92: { Do you still need this?
10 years ago (2012-09-13 23:01:28 UTC) #10
rsc
https://codereview.appspot.com/6498078/diff/8007/src/pkg/runtime/type.h File src/pkg/runtime/type.h (right): https://codereview.appspot.com/6498078/diff/8007/src/pkg/runtime/type.h#newcode92 src/pkg/runtime/type.h:92: { On 2012/09/13 23:01:29, iant wrote: > Do you ...
10 years ago (2012-09-14 02:46:28 UTC) #11
r
LGTM
10 years ago (2012-09-14 03:43:05 UTC) #12
rog
LGTM https://codereview.appspot.com/6498078/diff/8007/src/pkg/reflect/value.go File src/pkg/reflect/value.go (right): https://codereview.appspot.com/6498078/diff/8007/src/pkg/reflect/value.go#newcode1644 src/pkg/reflect/value.go:1644: SelectRecv // case <-Chan: nit: if there's a ...
10 years ago (2012-09-14 07:27:04 UTC) #13
rsc
Hello r@golang.org, iant@golang.org, rogpeppe@gmail.com (cc: golang-dev@googlegroups.com), Please take another look.
10 years ago (2012-09-17 21:34:37 UTC) #14
rsc
This is my last PTAL. I rewrote the test to be much cleaner.
10 years ago (2012-09-17 21:34:58 UTC) #15
bradfitz
On Mon, Sep 17, 2012 at 2:34 PM, Russ Cox <rsc@golang.org> wrote: > This is ...
10 years ago (2012-09-17 21:37:39 UTC) #16
rsc
On Mon, Sep 17, 2012 at 5:37 PM, Brad Fitzpatrick <bradfitz@golang.org> wrote: > On Mon, ...
10 years ago (2012-09-17 21:45:54 UTC) #17
iant
FYI https://codereview.appspot.com/6498078/diff/9/src/pkg/reflect/all_test.go File src/pkg/reflect/all_test.go (right): https://codereview.appspot.com/6498078/diff/9/src/pkg/reflect/all_test.go#newcode1071 src/pkg/reflect/all_test.go:1071: func TestSelect(t *testing.T) { I don't see a ...
10 years ago (2012-09-17 22:12:32 UTC) #18
r
LGTM after addressing iant's comments
10 years ago (2012-09-17 23:53:04 UTC) #19
rsc
10 years ago (2012-09-18 18:22:49 UTC) #20
*** Submitted as http://code.google.com/p/go/source/detail?r=1c33796be5cc ***

reflect: add Select

R=r, iant, rogpeppe, bradfitz
CC=golang-dev
http://codereview.appspot.com/6498078

http://codereview.appspot.com/6498078/diff/9/src/pkg/reflect/all_test.go
File src/pkg/reflect/all_test.go (right):

http://codereview.appspot.com/6498078/diff/9/src/pkg/reflect/all_test.go#newc...
src/pkg/reflect/all_test.go:1071: func TestSelect(t *testing.T) {
On 2012/09/17 22:12:32, iant wrote:
> I don't see a test for using reflect.Select with types larger than uintptr,
> although that is a slightly separate code path in the implementation.

Yes, and a buggy one. Added test and fixed.

http://codereview.appspot.com/6498078/diff/9/src/pkg/runtime/type.h
File src/pkg/runtime/type.h (right):

http://codereview.appspot.com/6498078/diff/9/src/pkg/runtime/type.h#newcode93
src/pkg/runtime/type.h:93: RecvDir = 1<<0,
On 2012/09/17 22:12:32, iant wrote:
> Still not needed, I think.

Deleted. And deleting them would have made me find the other bug much faster.
Nice call.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b