code review 8274043: net: delete DialOpt and Dialer; add struct Dialer

net: delete DialOpt and DialOption; add struct Dialer

Per discussions on golang-nuts and golang-dev:
"Some concerns with DialOpt"
https://groups.google.com/d/msg/golang-nuts/Hfh9aqhXyUw/W3uYi8lOdKcJ
https://groups.google.com/d/msg/golang-dev/37omSQeWv4Y/KASGIfPpXh0J

[bradfitz, 2013-04-02 18:58:15]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://go.googlecode.com/hg/

[julienschmidt, 2013-04-02 19:40:45]

This also fixes the timeout behavior, doesn't it?

In general this LGTM

https://codereview.appspot.com/8274043/diff/4001/src/pkg/net/dial_gen.go
File src/pkg/net/dial_gen.go (right):

https://codereview.appspot.com/8274043/diff/4001/src/pkg/net/dial_gen.go#newc...
src/pkg/net/dial_gen.go:17: func resolveAndDialChannel(net, addr string,
localAddr Addr, deadline time.Time) (Conn, error) {
Why do you pass a deadline here, when you internally calculate a timeout
anyways?

[r, 2013-04-02 19:42:53]

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

https://codereview.appspot.com/8274043/diff/4001/src/pkg/net/dial.go#newcode14
src/pkg/net/dial.go:14: // All fields may be their zero value.
maybe
// The zero value for each field is its default.

https://codereview.appspot.com/8274043/diff/4001/src/pkg/net/dial.go#newcode18
src/pkg/net/dial.go:18: // earlier.
default is ...

https://codereview.appspot.com/8274043/diff/4001/src/pkg/net/dial.go#newcode22
src/pkg/net/dial.go:22: // will fail. If Timeout is set, it may fail earlier.
default is...
(if it's zero, isn't the deadline in the past?)

https://codereview.appspot.com/8274043/diff/4001/src/pkg/net/dial.go#newcode28
src/pkg/net/dial.go:28: LocalAddr Addr
default is ...

https://codereview.appspot.com/8274043/diff/4001/src/pkg/net/dial.go#newcode110
src/pkg/net/dial.go:110: // by a colon and a protocol number or name and the
addr must be a
s/addr/address/ since you've done that for net.

https://codereview.appspot.com/8274043/diff/4001/src/pkg/net/dial_gen.go
File src/pkg/net/dial_gen.go (right):

https://codereview.appspot.com/8274043/diff/4001/src/pkg/net/dial_gen.go#newc...
src/pkg/net/dial_gen.go:15: // hasn't been pushed down into the pollserver.
(plan9 and some old
Plan 9

https://codereview.appspot.com/8274043/diff/4001/src/pkg/net/dial_gen.go#newc...
src/pkg/net/dial_gen.go:22: t := time.NewTimer(timeout)
if timeout is zero, why go through all this? instead of t.C below you could have
a channel variable that is either nil or t.C.

(is that a common case? if not, don't worry about it)

[bradfitz, 2013-04-02 19:55:53]

Hello golang-dev@googlegroups.com, google@julienschmidt.com, r@golang.org (cc:
golang-dev@googlegroups.com),

Please take another look.

[bradfitz, 2013-04-02 19:56:03]

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

https://codereview.appspot.com/8274043/diff/4001/src/pkg/net/dial.go#newcode14
src/pkg/net/dial.go:14: // All fields may be their zero value.
On 2013/04/02 19:42:53, r wrote:
> maybe
> // The zero value for each field is its default.

The default for plain func Dial?

Not sure how to say that.

Maybe:

// The zero value for each field is equivalent to dialing
// without any options, as done by the Dial function.

?

Not changed yet. Suggestions welcome.

https://codereview.appspot.com/8274043/diff/4001/src/pkg/net/dial.go#newcode18
src/pkg/net/dial.go:18: // earlier.
On 2013/04/02 19:42:53, r wrote:
> default is ...

I took a shot at all of them. Please critique.

https://codereview.appspot.com/8274043/diff/4001/src/pkg/net/dial.go#newcode22
src/pkg/net/dial.go:22: // will fail. If Timeout is set, it may fail earlier.
On 2013/04/02 19:42:53, r wrote:
> default is...
> (if it's zero, isn't the deadline in the past?)

Clarified. Maybe.

https://codereview.appspot.com/8274043/diff/4001/src/pkg/net/dial.go#newcode28
src/pkg/net/dial.go:28: LocalAddr Addr
On 2013/04/02 19:42:53, r wrote:
> default is ...

Done.

https://codereview.appspot.com/8274043/diff/4001/src/pkg/net/dial.go#newcode110
src/pkg/net/dial.go:110: // by a colon and a protocol number or name and the
addr must be a
On 2013/04/02 19:42:53, r wrote:
> s/addr/address/ since you've done that for net.

Done.

I only did s/net/network/ because everybody copies it in their packages and then
shadows the "net" package with a net variable, preventing them from wrapping
net.Dial in their Dial func.

But s/addr/address/ makes the text shorter too, so bonus.

https://codereview.appspot.com/8274043/diff/4001/src/pkg/net/dial_gen.go
File src/pkg/net/dial_gen.go (right):

https://codereview.appspot.com/8274043/diff/4001/src/pkg/net/dial_gen.go#newc...
src/pkg/net/dial_gen.go:15: // hasn't been pushed down into the pollserver.
(plan9 and some old
On 2013/04/02 19:42:53, r wrote:
> Plan 9

Done.

https://codereview.appspot.com/8274043/diff/4001/src/pkg/net/dial_gen.go#newc...
src/pkg/net/dial_gen.go:17: func resolveAndDialChannel(net, addr string,
localAddr Addr, deadline time.Time) (Conn, error) {
On 2013/04/02 19:40:45, julienschmidt wrote:
> Why do you pass a deadline here, when you internally calculate a timeout
> anyways?

so it matches the signature of dial and resolveAndDial.

https://codereview.appspot.com/8274043/diff/4001/src/pkg/net/dial_gen.go#newc...
src/pkg/net/dial_gen.go:22: t := time.NewTimer(timeout)
On 2013/04/02 19:42:53, r wrote:
> if timeout is zero, why go through all this? instead of t.C below you could
have
> a channel variable that is either nil or t.C.
> 
> (is that a common case? if not, don't worry about it)

if timeout is zero, the deadline has already expired and it should return an
error.  it's not common, and I don't want to duplicate the error logic up here,
so I'm just letting the code below handle returning the error.

[r, 2013-04-02 20:08:17]

LGTM with my few tweaks.

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

https://codereview.appspot.com/8274043/diff/11001/src/pkg/net/dial.go#newcode14
src/pkg/net/dial.go:14: // All fields may be their zero value.
an extended version of your rewrite:

// The zero value for each field is equivalent to dialing
// without that option. Dialing with the zero value of Dialer
// is therefore equivalent to just calling the Dial function.

https://codereview.appspot.com/8274043/diff/11001/src/pkg/net/dial.go#newcode30
src/pkg/net/dial.go:30: // like Timeout.
"as with the Timeout option."

https://codereview.appspot.com/8274043/diff/11001/src/pkg/net/dial.go#newcode41
src/pkg/net/dial.go:41: // Or zero, if neither are set.
s/are/is/

[bradfitz, 2013-04-02 20:24:19]

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

net: delete DialOpt and DialOption; add struct Dialer

Per discussions on golang-nuts and golang-dev:
"Some concerns with DialOpt"
https://groups.google.com/d/msg/golang-nuts/Hfh9aqhXyUw/W3uYi8lOdKcJ
https://groups.google.com/d/msg/golang-dev/37omSQeWv4Y/KASGIfPpXh0J

R=golang-dev, google, r
CC=golang-dev
https://codereview.appspot.com/8274043

[rog, 2013-04-03 13:41:52]

much nicer. thanks a lot for pushing this change.