code review 12662043: net: avoid string operation and make valid domain names...

net: avoid string operation and make valid domain names explicit

Having a trailing dot in the string doesn't really simplify
the checking loop in isDomainName. Avoid this unnecessary allocation.
Also make the valid domain names more explicit by adding some more
test cases.

benchmark            old ns/op    new ns/op    delta
BenchmarkDNSNames       2420.0        983.0  -59.38%

benchmark           old allocs   new allocs    delta
BenchmarkDNSNames           12            0  -100.00%

benchmark            old bytes    new bytes    delta
BenchmarkDNSNames          336            0  -100.00%

[volker.dobler, 2013-08-08 13:32:18]

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/

[bradfitz, 2013-08-08 18:03:46]

https://codereview.appspot.com/12662043/diff/4005/src/pkg/net/dnsname_test.go
File src/pkg/net/dnsname_test.go (right):

https://codereview.appspot.com/12662043/diff/4005/src/pkg/net/dnsname_test.go...
src/pkg/net/dnsname_test.go:71: s16 := "0123456789abcdef"
drop all this

https://codereview.appspot.com/12662043/diff/4005/src/pkg/net/dnsname_test.go...
src/pkg/net/dnsname_test.go:76: {s63, true},
and just say strings.Repeat("a", 63)

https://codereview.appspot.com/12662043/diff/4005/src/pkg/net/dnsname_test.go...
src/pkg/net/dnsname_test.go:80: for i := range benchmarks {
for i, tt := range benchmarks {

https://codereview.appspot.com/12662043/diff/4005/src/pkg/net/dnsname_test.go...
src/pkg/net/dnsname_test.go:81: if isDomainName(benchmarks[i].name) !=
benchmarks[i].result {
tt.name and tt.result

https://codereview.appspot.com/12662043/diff/4005/src/pkg/net/dnsname_test.go...
src/pkg/net/dnsname_test.go:82: b.Fatalf("isDomainName(%q)", benchmarks[i].name)
tt.name

[volker.dobler, 2013-08-08 22:08:08]

PTAL

https://codereview.appspot.com/12662043/diff/4005/src/pkg/net/dnsname_test.go
File src/pkg/net/dnsname_test.go (right):

https://codereview.appspot.com/12662043/diff/4005/src/pkg/net/dnsname_test.go...
src/pkg/net/dnsname_test.go:71: s16 := "0123456789abcdef"
On 2013/08/08 18:03:46, bradfitz wrote:
> drop all this

Done.

https://codereview.appspot.com/12662043/diff/4005/src/pkg/net/dnsname_test.go...
src/pkg/net/dnsname_test.go:76: {s63, true},
On 2013/08/08 18:03:46, bradfitz wrote:
> and just say strings.Repeat("a", 63)

Done.

https://codereview.appspot.com/12662043/diff/4005/src/pkg/net/dnsname_test.go...
src/pkg/net/dnsname_test.go:80: for i := range benchmarks {
On 2013/08/08 18:03:46, bradfitz wrote:
> for i, tt := range benchmarks {

Done.

https://codereview.appspot.com/12662043/diff/4005/src/pkg/net/dnsname_test.go...
src/pkg/net/dnsname_test.go:81: if isDomainName(benchmarks[i].name) !=
benchmarks[i].result {
On 2013/08/08 18:03:46, bradfitz wrote:
> tt.name and tt.result

Done.

https://codereview.appspot.com/12662043/diff/4005/src/pkg/net/dnsname_test.go...
src/pkg/net/dnsname_test.go:82: b.Fatalf("isDomainName(%q)", benchmarks[i].name)
On 2013/08/08 18:03:46, bradfitz wrote:
> tt.name

Done.

[bradfitz, 2013-08-08 22:23:26]

https://codereview.appspot.com/12662043/diff/1002/src/pkg/net/dnsname_test.go
File src/pkg/net/dnsname_test.go (right):

https://codereview.appspot.com/12662043/diff/1002/src/pkg/net/dnsname_test.go...
src/pkg/net/dnsname_test.go:79: b.Fatalf("isDomainName(%q)", tc.name)
Errorf, not Fatalf.

Also: "isDomainName(%q) = %v; want %v", tc.name, !tc.result, tc.result

[volker.dobler, 2013-08-08 23:21:40]

PTAL

https://codereview.appspot.com/12662043/diff/1002/src/pkg/net/dnsname_test.go
File src/pkg/net/dnsname_test.go (right):

https://codereview.appspot.com/12662043/diff/1002/src/pkg/net/dnsname_test.go...
src/pkg/net/dnsname_test.go:79: b.Fatalf("isDomainName(%q)", tc.name)
On 2013/08/08 22:23:26, bradfitz wrote:
> Errorf, not Fatalf.
> 
> Also: "isDomainName(%q) = %v; want %v", tc.name, !tc.result, tc.result

Done. But...

I know: The "got; want" pattern, but isn't this a bit strange
for bools? If the got value is wrong it is pretty clear what the
want value is, or? We are not doing ternary or intuitionistic logic
here.

Fatalf seems to be more common
~/go/src/pkg $ grep b.Fatalf `find . -name "*_test.go"` | wc -l
35
~/go/src/pkg $ $ grep b.Errorf `find . -name "*_test.go"` | wc -l
9
and more logical: There is no point in finishing the benchmark of
a buggy function.

[bradfitz, 2013-08-08 23:29:13]

Let's say I make a change.

I break something.

I'd rather see the 7 cases I broke (and maybe see a pattern of how I broke
it, since humans are good at seeing patterns), rather than seeing just one
error.

As for the got; want pattern--- if you fail in the same way, I don't have
to guess what "foo(%q) doesn't work!" means.  I don't want to go read the
test case lines and figure out what the function was supposed to do.

Both points are about saving the debugger (probably not you) time later, by
the author (you) paying the cost now.






On Thu, Aug 8, 2013 at 4:21 PM, <dr.volker.dobler@gmail.com> wrote:

> PTAL
>
>
>
>
> https://codereview.appspot.**com/12662043/diff/1002/src/**
>
pkg/net/dnsname_test.go<https://codereview.appspot.com/12662043/diff/1002/src/pkg/net/dnsname_test.go>
> File src/pkg/net/dnsname_test.go (right):
>
> https://codereview.appspot.**com/12662043/diff/1002/src/**
>
pkg/net/dnsname_test.go#**newcode79<https://codereview.appspot.com/12662043/diff/1002/src/pkg/net/dnsname_test.go#newcode79>
> src/pkg/net/dnsname_test.go:**79: b.Fatalf("isDomainName(%q)", tc.name)
> On 2013/08/08 22:23:26, bradfitz wrote:
>
>> Errorf, not Fatalf.
>>
>
>  Also: "isDomainName(%q) = %v; want %v", tc.name, !tc.result, tc.result
>>
>
> Done. But...
>
> I know: The "got; want" pattern, but isn't this a bit strange
> for bools? If the got value is wrong it is pretty clear what the
> want value is, or? We are not doing ternary or intuitionistic logic
> here.
>
> Fatalf seems to be more common
> ~/go/src/pkg $ grep b.Fatalf `find . -name "*_test.go"` | wc -l
> 35
> ~/go/src/pkg $ $ grep b.Errorf `find . -name "*_test.go"` | wc -l
> 9
> and more logical: There is no point in finishing the benchmark of
> a buggy function.
>
>
https://codereview.appspot.**com/12662043/<https://codereview.appspot.com/126...
>

[bradfitz, 2013-08-08 23:33:49]

LGTM



On Thu, Aug 8, 2013 at 4:29 PM, Brad Fitzpatrick <bradfitz@golang.org>wrote:

> Let's say I make a change.
>
> I break something.
>
> I'd rather see the 7 cases I broke (and maybe see a pattern of how I broke
> it, since humans are good at seeing patterns), rather than seeing just one
> error.
>
> As for the got; want pattern--- if you fail in the same way, I don't have
> to guess what "foo(%q) doesn't work!" means.  I don't want to go read the
> test case lines and figure out what the function was supposed to do.
>
> Both points are about saving the debugger (probably not you) time later,
> by the author (you) paying the cost now.
>
>
>
>
>
>
> On Thu, Aug 8, 2013 at 4:21 PM, <dr.volker.dobler@gmail.com> wrote:
>
>> PTAL
>>
>>
>>
>>
>> https://codereview.appspot.**com/12662043/diff/1002/src/**
>>
pkg/net/dnsname_test.go<https://codereview.appspot.com/12662043/diff/1002/src/pkg/net/dnsname_test.go>
>> File src/pkg/net/dnsname_test.go (right):
>>
>> https://codereview.appspot.**com/12662043/diff/1002/src/**
>>
pkg/net/dnsname_test.go#**newcode79<https://codereview.appspot.com/12662043/diff/1002/src/pkg/net/dnsname_test.go#newcode79>
>> src/pkg/net/dnsname_test.go:**79: b.Fatalf("isDomainName(%q)", tc.name)
>> On 2013/08/08 22:23:26, bradfitz wrote:
>>
>>> Errorf, not Fatalf.
>>>
>>
>>  Also: "isDomainName(%q) = %v; want %v", tc.name, !tc.result, tc.result
>>>
>>
>> Done. But...
>>
>> I know: The "got; want" pattern, but isn't this a bit strange
>> for bools? If the got value is wrong it is pretty clear what the
>> want value is, or? We are not doing ternary or intuitionistic logic
>> here.
>>
>> Fatalf seems to be more common
>> ~/go/src/pkg $ grep b.Fatalf `find . -name "*_test.go"` | wc -l
>> 35
>> ~/go/src/pkg $ $ grep b.Errorf `find . -name "*_test.go"` | wc -l
>> 9
>> and more logical: There is no point in finishing the benchmark of
>> a buggy function.
>>
>>
https://codereview.appspot.**com/12662043/<https://codereview.appspot.com/126...
>>
>
>

[bradfitz, 2013-08-08 23:33:59]

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

net: avoid string operation and make valid domain names explicit

Having a trailing dot in the string doesn't really simplify
the checking loop in isDomainName. Avoid this unnecessary allocation.
Also make the valid domain names more explicit by adding some more
test cases.

benchmark            old ns/op    new ns/op    delta
BenchmarkDNSNames       2420.0        983.0  -59.38%

benchmark           old allocs   new allocs    delta
BenchmarkDNSNames           12            0  -100.00%

benchmark            old bytes    new bytes    delta
BenchmarkDNSNames          336            0  -100.00%

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

Committer: Brad Fitzpatrick <bradfitz@golang.org>