code review 5753045: net: do not use reflect for DNS messages.

net: do not use reflect for DNS messages.

Fixes issue 3201.

[remyoudompheng, 2012-03-05 23:58:50]

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

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

[remyoudompheng, 2012-03-05 23:59:22]

This is essentially a request for comments. I am not sure at all whether the
tests are complete enough.

[bradfitzgoog, 2012-03-06 00:02:59]

Fun approach.

Russ was going to start on this later tonight, but it looks like you're
about done.  Want to finish up the rest so reflect totally goes away?


On Mon, Mar 5, 2012 at 3:59 PM, <remyoudompheng@gmail.com> wrote:

> This is essentially a request for comments. I am not sure at all whether
> the tests are complete enough.
>
>
http://codereview.appspot.com/**5753045/<http://codereview.appspot.com/5753045/>
>

[remyoudompheng, 2012-03-06 00:08:05]

On 2012/03/06 00:02:59, bradfitzgoog wrote:
> Fun approach.

No problem if you indeed find it fun.

[kevlar, 2012-03-06 00:27:01]

FYI

On 2012/03/06 00:08:05, remyoudompheng wrote:
> On 2012/03/06 00:02:59, bradfitzgoog wrote:
> > Fun approach.
> 
> No problem if you indeed find it fun.

Clever!  I'm curious whether this is faster than reflect (well, actually, I
assume it is I'm just curious by how much).

[remyoudompheng, 2012-03-06 00:27:14]

Hello golang-dev@googlegroups.com, bradfitz@google.com (cc:
golang-dev@googlegroups.com, remy@archlinux.org),

Please take another look.

[bradfitz, 2012-03-06 00:45:53]

Nice.

Few minor things.  Wait for rsc or r to review.

http://codereview.appspot.com/5753045/diff/1002/src/pkg/net/dnsmsg.go
File src/pkg/net/dnsmsg.go (right):

http://codereview.appspot.com/5753045/diff/1002/src/pkg/net/dnsmsg.go#newcode10
src/pkg/net/dnsmsg.go:10: // Each message structure has a Walk method that
allows to use
s/allows to use/allows use of/
or better:
s/allows to use/that is used by/

http://codereview.appspot.com/5753045/diff/1002/src/pkg/net/dnsmsg.go#newcode74
src/pkg/net/dnsmsg.go:74: Walk(func(v interface{}, name, tag string) (ok bool))
(ok bool)
describe what v is (pointer to a struct field? except in the case of []byte?)

name is pretty obvious, but tag isn't, so might as well describe all three.

and when is ok false?

http://codereview.appspot.com/5753045/diff/1002/src/pkg/net/dnsmsg_test.go
File src/pkg/net/dnsmsg_test.go (right):

http://codereview.appspot.com/5753045/diff/1002/src/pkg/net/dnsmsg_test.go#ne...
src/pkg/net/dnsmsg_test.go:23: t.Log(msg.String()) // exercise this code path
I had this too, but found it a little spammy and kept it at just "msg.String()"
without side-effects.

http://codereview.appspot.com/5753045/diff/1002/src/pkg/net/dnsmsg_test.go#ne...
src/pkg/net/dnsmsg_test.go:43: // repack and unpack.
nice.

http://codereview.appspot.com/5753045/diff/1002/src/pkg/net/dnsmsg_test.go#ne...
src/pkg/net/dnsmsg_test.go:65: t.Log(msg.String()) // exercise this code path
spam?

[bradfitz, 2012-03-06 00:47:56]

On Mon, Mar 5, 2012 at 4:27 PM, <kevlar@google.com> wrote:

> FYI
>
>
> On 2012/03/06 00:08:05, remyoudompheng wrote:
>
>> On 2012/03/06 00:02:59, bradfitzgoog wrote:
>> > Fun approach.
>>
>
>  No problem if you indeed find it fun.
>>
>
> Clever!  I'm curious whether this is faster than reflect (well,
> actually, I assume it is I'm just curious by how much).
>

 Faster or slower, the net package is getting slimmer.

[rsc1, 2012-03-06 02:51:28]

LGTM

So much better than what I was planning.
Thanks very much.

I agree with Brad about the log spam.

[bradfitz, 2012-03-06 04:54:27]

Also, you can now update this new file in this CL,

src/pkg/go/build/deps_test.go

... and remove the "net": ... "reflect" bit and update the TODO.


On Mon, Mar 5, 2012 at 4:27 PM, <remyoudompheng@gmail.com> wrote:

> Hello golang-dev@googlegroups.com, bradfitz@google.com (cc:
> golang-dev@googlegroups.com, remy@archlinux.org),
>
> Please take another look.
>
>
>
http://codereview.appspot.com/**5753045/<http://codereview.appspot.com/5753045/>
>

[remyoudompheng, 2012-03-06 06:56:18]

Hello bradfitz@google.com, bradfitz@golang.org, rsc@google.com (cc:
golang-dev@googlegroups.com, remy@archlinux.org),

Please take another look.

[bradfitz, 2012-03-06 06:59:13]

LGTM

Thanks!

http://codereview.appspot.com/5753045/diff/5005/src/pkg/go/build/deps_test.go
File src/pkg/go/build/deps_test.go (right):

http://codereview.appspot.com/5753045/diff/5005/src/pkg/go/build/deps_test.go...
src/pkg/go/build/deps_test.go:229: // TODO: Remove math/rand of possible.
// TODO: maybe remove math/rand
or:
// TODO: remove math/rand?

[remyoudompheng, 2012-03-06 06:59:58]

> Also, you can now update this new file in this CL,
> 
> src/pkg/go/build/deps_test.go
> 
> ... and remove the "net": ... "reflect" bit and update the TODO.
> 

Done.

http://codereview.appspot.com/5753045/diff/1002/src/pkg/net/dnsmsg.go
File src/pkg/net/dnsmsg.go (right):

http://codereview.appspot.com/5753045/diff/1002/src/pkg/net/dnsmsg.go#newcode10
src/pkg/net/dnsmsg.go:10: // Each message structure has a Walk method that
allows to use
On 2012/03/06 00:45:53, bradfitz wrote:
> s/allows to use/allows use of/
> or better:
> s/allows to use/that is used by/

Done.

http://codereview.appspot.com/5753045/diff/1002/src/pkg/net/dnsmsg.go#newcode74
src/pkg/net/dnsmsg.go:74: Walk(func(v interface{}, name, tag string) (ok bool))
(ok bool)
On 2012/03/06 00:45:53, bradfitz wrote:
> describe what v is (pointer to a struct field? except in the case of []byte?)
> 
> name is pretty obvious, but tag isn't, so might as well describe all three.
> 
> and when is ok false?

Done.

http://codereview.appspot.com/5753045/diff/1002/src/pkg/net/dnsmsg_test.go
File src/pkg/net/dnsmsg_test.go (right):

http://codereview.appspot.com/5753045/diff/1002/src/pkg/net/dnsmsg_test.go#ne...
src/pkg/net/dnsmsg_test.go:23: t.Log(msg.String()) // exercise this code path
On 2012/03/06 00:45:53, bradfitz wrote:
> I had this too, but found it a little spammy and kept it at just
"msg.String()"
> without side-effects.

Done.

http://codereview.appspot.com/5753045/diff/1002/src/pkg/net/dnsmsg_test.go#ne...
src/pkg/net/dnsmsg_test.go:65: t.Log(msg.String()) // exercise this code path
On 2012/03/06 00:45:53, bradfitz wrote:
> spam?

Done.

[remyoudompheng, 2012-03-06 07:01:08]

Hello bradfitz@google.com, bradfitz@golang.org, rsc@google.com (cc:
golang-dev@googlegroups.com, remy@archlinux.org),

Please take another look.

[bradfitz, 2012-03-06 07:01:47]

LGTM

On Mon, Mar 5, 2012 at 11:01 PM, <remyoudompheng@gmail.com> wrote:

> Hello bradfitz@google.com, bradfitz@golang.org, rsc@google.com (cc:
> golang-dev@googlegroups.com, remy@archlinux.org),
>
> Please take another look.
>
>
>
http://codereview.appspot.com/**5753045/<http://codereview.appspot.com/5753045/>
>

[remyoudompheng, 2012-03-06 07:03:10]

*** Submitted as http://code.google.com/p/go/source/detail?r=c1f5756f94b0 ***

net: do not use reflect for DNS messages.

Fixes issue 3201.

R=bradfitz, bradfitz, rsc
CC=golang-dev, remy
http://codereview.appspot.com/5753045