code review 4079053: replace non-blocking send, receive syntax with select

replace non-blocking send, receive syntax with select

[rsc, 2011-01-30 22:46:56]

Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com),

I'd like you to review this change.

[nigeltao, 2011-01-30 23:49:42]

http://codereview.appspot.com/4079053/diff/30/src/pkg/exp/draw/x11/conn.go
File src/pkg/exp/draw/x11/conn.go (right):

http://codereview.appspot.com/4079053/diff/30/src/pkg/exp/draw/x11/conn.go#ne...
src/pkg/exp/draw/x11/conn.go:127: // sent flush notification
Should comments be complete sentences?

[rsc, 2011-01-31 00:08:17]

http://codereview.appspot.com/4079053/diff/30/src/pkg/exp/draw/x11/conn.go
File src/pkg/exp/draw/x11/conn.go (right):

http://codereview.appspot.com/4079053/diff/30/src/pkg/exp/draw/x11/conn.go#ne...
src/pkg/exp/draw/x11/conn.go:127: // sent flush notification
On 2011/01/30 23:49:42, nigeltao wrote:
> Should comments be complete sentences?

Doc comments, yes.  Long comments, yes.
The rest, not necessary.

That said, you wrote the original code,
and it's true that these fragments don't
match the rest of the file.  Expanded.

[niemeyer, 2011-01-31 18:02:02]

LGTM

Some minor comments:

http://codereview.appspot.com/4079053/diff/7029/src/pkg/path/path_test.go
File src/pkg/path/path_test.go (right):

http://codereview.appspot.com/4079053/diff/7029/src/pkg/path/path_test.go#new...
src/pkg/path/path_test.go:287: t.Errorf("%d. error expected, none found", i)
FWIW, the original code would break the outside loop here.

In the worst case it will just error twice, but shouldn't
be a big deal either way.

http://codereview.appspot.com/4079053/diff/7029/src/pkg/rpc/server_test.go
File src/pkg/rpc/server_test.go (right):

http://codereview.appspot.com/4079053/diff/7029/src/pkg/rpc/server_test.go#ne...
src/pkg/rpc/server_test.go:371: t.Fatal("deadlock")
Much better.

http://codereview.appspot.com/4079053/diff/7029/test/chan/nonblock.go
File test/chan/nonblock.go (right):

http://codereview.appspot.com/4079053/diff/7029/test/chan/nonblock.go#newcode125
test/chan/nonblock.go:125: sleep()
Might use <-time.After(...) in all of these blocks as well?

[rsc, 2011-01-31 18:07:54]

nice catch; updated

http://codereview.appspot.com/4079053/diff/7029/src/pkg/path/path_test.go
File src/pkg/path/path_test.go (right):

http://codereview.appspot.com/4079053/diff/7029/src/pkg/path/path_test.go#new...
src/pkg/path/path_test.go:287: t.Errorf("%d. error expected, none found", i)
On 2011/01/31 18:02:02, niemeyer wrote:
> FWIW, the original code would break the outside loop here.
> 
> In the worst case it will just error twice, but shouldn't
> be a big deal either way.

Done.

http://codereview.appspot.com/4079053/diff/7029/test/chan/nonblock.go
File test/chan/nonblock.go (right):

http://codereview.appspot.com/4079053/diff/7029/test/chan/nonblock.go#newcode125
test/chan/nonblock.go:125: sleep()
On 2011/01/31 18:02:02, niemeyer wrote:
> Might use <-time.After(...) in all of these blocks as well?

Rather not.  Not trying to rewrite code here.

[r, 2011-01-31 21:04:45]

LGTM

http://codereview.appspot.com/4079053/diff/12001/src/pkg/fmt/print.go
File src/pkg/fmt/print.go (right):

http://codereview.appspot.com/4079053/diff/12001/src/pkg/fmt/print.go#newcode77
src/pkg/fmt/print.go:77: type cache struct {
put a comment on this type

http://codereview.appspot.com/4079053/diff/12001/src/pkg/fmt/print.go#newcode108
src/pkg/fmt/print.go:108: // Allocate a new pp struct.  Probably can grab the
previous one from ppFree.
this comment is now inaccurate

http://codereview.appspot.com/4079053/diff/12001/src/pkg/fmt/scan.go
File src/pkg/fmt/scan.go (right):

http://codereview.appspot.com/4079053/diff/12001/src/pkg/fmt/scan.go#newcode308
src/pkg/fmt/scan.go:308: // Allocate a new ss struct.  Probably can grab the
previous one from ssFree.
again, inaccurate

http://codereview.appspot.com/4079053/diff/12001/test/chan/perm.go
File test/chan/perm.go (right):

http://codereview.appspot.com/4079053/diff/12001/test/chan/perm.go#newcode25
test/chan/perm.go:25: //TODO(rsc): uncomment
should say a little more
// TODO(rsc): uncomment when ok reports "closed"

http://codereview.appspot.com/4079053/diff/12001/test/fixedbugs/bug069.go
File test/fixedbugs/bug069.go (right):

http://codereview.appspot.com/4079053/diff/12001/test/fixedbugs/bug069.go#new...
test/fixedbugs/bug069.go:10: //TODO(rsc): uncomment
ditto

http://codereview.appspot.com/4079053/diff/12001/test/fixedbugs/bug196.go
File test/fixedbugs/bug196.go (right):

http://codereview.appspot.com/4079053/diff/12001/test/fixedbugs/bug196.go#new...
test/fixedbugs/bug196.go:16: //TODO(rsc): uncomment
ditto

http://codereview.appspot.com/4079053/diff/12001/test/fixedbugs/bug234.go
File test/fixedbugs/bug234.go (right):

http://codereview.appspot.com/4079053/diff/12001/test/fixedbugs/bug234.go#new...
test/fixedbugs/bug234.go:10: //TODO(rsc): uncomment
ditto

http://codereview.appspot.com/4079053/diff/12001/test/fixedbugs/bug242.go
File test/fixedbugs/bug242.go (right):

http://codereview.appspot.com/4079053/diff/12001/test/fixedbugs/bug242.go#new...
test/fixedbugs/bug242.go:104: //TODO(rsc): uncomment
ditto

http://codereview.appspot.com/4079053/diff/12001/test/named1.go
File test/named1.go (right):

http://codereview.appspot.com/4079053/diff/12001/test/named1.go#newcode46
test/named1.go:46: ////TODO(rsc): uncomment and s/e/E/
ditto

[rsc, 2011-01-31 23:36:30]

*** Submitted as 413e7fa8ef3d ***

replace non-blocking send, receive syntax with select

R=golang-dev, nigeltao, niemeyer, r
CC=golang-dev
http://codereview.appspot.com/4079053