It's so easy to implement nil channels I think it should be done. That means ...
11 years, 7 months ago
(2012-09-05 15:56:05 UTC)
#4
It's so easy to implement nil channels I think it should be done. That
means that reflect can implement the full semantics of the select
statement.
If you don't implement it here, it means in Go it's silently ignored
and in reflect it panics. That's a startling distinction.
-rob
On Wed, Sep 5, 2012 at 11:56 AM, Rob Pike <r@golang.org> wrote: > It's so ...
11 years, 7 months ago
(2012-09-05 21:41:59 UTC)
#5
On Wed, Sep 5, 2012 at 11:56 AM, Rob Pike <r@golang.org> wrote:
> It's so easy to implement nil channels I think it should be done. That
> means that reflect can implement the full semantics of the select
> statement.
>
> If you don't implement it here, it means in Go it's silently ignored
> and in reflect it panics. That's a startling distinction.
nil channels are already handled correctly by the select runtime proper.
What you're suggesting is to treat a zero reflect.Value as equivalent to
a nil channel. I can be talked into that, but the code as it stands is
completely
consistent with the language. This is a new opportunity created by the
reflect API, yet another way to say 'no value here'.
Russ
It's great to see this going in. I have a reservation about the API, though. ...
11 years, 7 months ago
(2012-09-10 10:11:40 UTC)
#7
It's great to see this going in.
I have a reservation about the API, though. I think that
interpreting the zero value of SelectCase as the default
clause is perhaps a bit dangerous.
For instance, here is a way of specifying a select statement
that avoids the need to refer to case clauses numerically:
const (
muxCase = iota
closedCase
)
cases := []reflect.SelectCase{
muxCase: {
Dir: reflect.RecvDir,
Chan: someC,
},
closedCase: {
Dir: reflect.RecvDir,
Chan: someOtherC,
},
}
switch chosen, v, ok := reflect.Select(cases); chosen {
case muxCase:
...
case ...
}
The problem is, if you add a constant, you can
easily leave a blank entry in the SelectCase slice, and
that will make the select non-blocking - a fairly significant
change and not easy to see in the code.
How about adding an explicit non-zero constant
for the default case (reflect.Default perhaps) and making
the select logic ignore cases with a zero Dir?
Then it's easier to dynamically add and remove cases
from the SelectCase slice, we can continue to
enforce IsValid on channels that *do* have a direction,
and it's clearer to the reader of the code when a select
is non-blocking.
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?
11 years, 7 months ago
(2012-09-13 23:01:28 UTC)
#10
On Mon, Sep 17, 2012 at 5:37 PM, Brad Fitzpatrick <bradfitz@golang.org> wrote: > On Mon, ...
11 years, 7 months ago
(2012-09-17 21:45:54 UTC)
#17
On Mon, Sep 17, 2012 at 5:37 PM, Brad Fitzpatrick <bradfitz@golang.org> wrote:
> On Mon, Sep 17, 2012 at 2:34 PM, Russ Cox <rsc@golang.org> wrote:
>> This is my last PTAL.
>
> After this one there is no "Please".
I just figured everyone was tired of LGTMing this CL. But I like your
interpretation too.
Issue 6498078: code review 6498078: reflect: add Select
(Closed)
Created 11 years, 7 months ago by rsc
Modified 11 years, 7 months ago
Reviewers:
Base URL:
Comments: 30
