code review 7300065: net: simplify Dial, Listen, ListenPacket and those helpers

net: simplify Dial, Listen, ListenPacket and those helpers

[mikio, 2013-02-08 01:51:48]

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

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

[dave_cheney.net, 2013-02-08 03:35:06]

Thank you. I think this is very close but I would like to do another round of
reviews and get feedback from others as this is core stuff.

https://codereview.appspot.com/7300065/diff/5001/src/pkg/net/dial.go
File src/pkg/net/dial.go (right):

https://codereview.appspot.com/7300065/diff/5001/src/pkg/net/dial.go#newcode77
src/pkg/net/dial.go:77: func Dial(net, addr string) (Conn, error) {
I preferred the old name, resolveDialAddr("dial", ...) stutters. This is not a
strong objection.

https://codereview.appspot.com/7300065/diff/5001/src/pkg/net/dial.go#newcode172
src/pkg/net/dial.go:172: switch la := a.(type) {
can reuse a here if you like

switch a := a.(type) { ... }

https://codereview.appspot.com/7300065/diff/5001/src/pkg/net/dial.go#newcode185
src/pkg/net/dial.go:185: a, err := resolveDialAddr("listen", net, laddr,
noDeadline)
resolveDialAddr("listen", ... ) doesn't read as well as resolveNetAddr("listen",
... ), as in "Resolve Net Addr for Listen"

https://codereview.appspot.com/7300065/diff/5001/src/pkg/net/dial.go#newcode189
src/pkg/net/dial.go:189: switch la := a.(type) {
same as L172

https://codereview.appspot.com/7300065/diff/5001/src/pkg/net/fd_windows.go
File src/pkg/net/fd_windows.go (right):

https://codereview.appspot.com/7300065/diff/5001/src/pkg/net/fd_windows.go#ne...
src/pkg/net/fd_windows.go:62: }
It would be awesome if dialTimeout could be shared across both unix and windows.

[bradfitz, 2013-02-08 03:45:16]

https://codereview.appspot.com/7300065/diff/5001/src/pkg/net/dial.go
File src/pkg/net/dial.go (right):

https://codereview.appspot.com/7300065/diff/5001/src/pkg/net/dial.go#newcode9
src/pkg/net/dial.go:9: func parseDialNetwork(net string) (string, int, error) {
named result parameters read WAY nicer.  Please keep them.

[mikio, 2013-02-08 03:45:34]

> I preferred the old name, resolveDialAddr("dial", ...) stutters.

thanks. as you know I'm not good at naming things.

hm, in that case do you have any good candidate for 
parseDialNetwork too?

[dave_cheney.net, 2013-02-08 03:53:55]

> hm, in that case do you have any good candidate for
> parseDialNetwork too?

parseNetwork(net string) (afnet string, proto int, err error),  sounds
reasonable,

Although I'm not a fan of named returns in general I agree with Brad
in this case. Possibly if the function returned specific types rather
than strings and int the names could be omitted. Ultimately this is an
internal function and it's probably not worth overthinking it.

[mikio, 2013-02-08 04:10:55]

> parseNetwork(net string) (afnet string, proto int, err error),  sounds
reasonable,

thanks. i think i will go with parseNetwork and resolveAddr.

> Although I'm not a fan of named returns in general I agree with Brad in this
case.

ack, i was just afraid of the shadow of shadowing.

[mikio, 2013-02-08 11:14:11]

Hello golang-dev@googlegroups.com, dave@cheney.net, bradfitz@golang.org (cc:
golang-dev@googlegroups.com),

Please take another look.

[mikio, 2013-02-08 11:14:36]

https://codereview.appspot.com/7300065/diff/5001/src/pkg/net/dial.go
File src/pkg/net/dial.go (right):

https://codereview.appspot.com/7300065/diff/5001/src/pkg/net/dial.go#newcode9
src/pkg/net/dial.go:9: func parseDialNetwork(net string) (string, int, error) {
On 2013/02/08 03:45:16, bradfitz wrote:
> named result parameters read WAY nicer.  Please keep them.

Done.

https://codereview.appspot.com/7300065/diff/5001/src/pkg/net/dial.go#newcode77
src/pkg/net/dial.go:77: func Dial(net, addr string) (Conn, error) {
On 2013/02/08 03:35:06, dfc wrote:
> I preferred the old name, resolveDialAddr("dial", ...) stutters. This is not a
> strong objection.

changed to resolveAddr

https://codereview.appspot.com/7300065/diff/5001/src/pkg/net/dial.go#newcode172
src/pkg/net/dial.go:172: switch la := a.(type) {
On 2013/02/08 03:35:06, dfc wrote:
> can reuse a here if you like
> 
> switch a := a.(type) { ... }

Done.

https://codereview.appspot.com/7300065/diff/5001/src/pkg/net/dial.go#newcode185
src/pkg/net/dial.go:185: a, err := resolveDialAddr("listen", net, laddr,
noDeadline)
On 2013/02/08 03:35:06, dfc wrote:
> resolveDialAddr("listen", ... ) doesn't read as well as
resolveNetAddr("listen",
> ... ), as in "Resolve Net Addr for Listen"

Done.

https://codereview.appspot.com/7300065/diff/5001/src/pkg/net/dial.go#newcode189
src/pkg/net/dial.go:189: switch la := a.(type) {
On 2013/02/08 03:35:06, dfc wrote:
> same as L172

Done.

[dave_cheney.net, 2013-02-08 12:09:10]

LGTM. Thank you.

A few notes for extra credit.

38      func resolveAddr(op, net, addr string, deadline time.Time) (Addr, error)
{
39              afnet, _, err := parseNetwork(net)
40              if err != nil {
41 MISS                 return nil, &OpError{op, net, nil, err}
42              }
43              if op == "dial" && addr == "" {
44 MISS                 return nil, &OpError{op, net, nil, errMissingAddress}
45              }
46              switch afnet {
47              case "unix", "unixgram", "unixpacket":
48                      return ResolveUnixAddr(afnet, addr)
49              }
50              return resolveInternetAddr(afnet, addr, deadline)
51      }

Would it be possible to add a test case to exercise line 44 ?

[mikio, 2013-02-08 12:31:06]

> A few notes for extra credit.

gocov?

[dave_cheney.net, 2013-02-08 12:32:12]

Yup.

On Fri, Feb 8, 2013 at 11:31 PM, Mikio Hara <mikioh.mikioh@gmail.com> wrote:
>> A few notes for extra credit.
>
> gocov?

[mikio, 2013-02-08 12:51:08]

thanks. i will add that in following CLs.

[mikio, 2013-02-08 12:53:30]

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

net: simplify Dial, Listen, ListenPacket and those helpers

R=golang-dev, dave, bradfitz
CC=golang-dev
https://codereview.appspot.com/7300065