Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(3394)

Issue 12662043: code review 12662043: net: avoid string operation and make valid domain names... (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
10 years, 7 months ago by volker.dobler
Modified:
10 years, 7 months ago
Reviewers:
bradfitz
CC:
golang-dev, bradfitz
Visibility:
Public.

Description

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%

Patch Set 1 #

Patch Set 2 : diff -r 2a74ed09bf00 https://code.google.com/p/go/ #

Patch Set 3 : diff -r 2a74ed09bf00 https://code.google.com/p/go/ #

Total comments: 10

Patch Set 4 : diff -r b0240b16a8e0 https://code.google.com/p/go/ #

Total comments: 2

Patch Set 5 : diff -r b0240b16a8e0 https://code.google.com/p/go/ #

Unified diffs Side-by-side diffs Delta from patch set Stats (+25 lines, -7 lines) Patch
M src/pkg/net/dnsclient.go View 1 3 chunks +6 lines, -6 lines 0 comments Download
M src/pkg/net/dnsname_test.go View 1 2 3 4 4 chunks +19 lines, -1 line 0 comments Download

Messages

Total messages: 8
volker.dobler
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/
10 years, 7 months ago (2013-08-08 13:32:18 UTC) #1
bradfitz
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#newcode71 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#newcode76 src/pkg/net/dnsname_test.go:76: {s63, ...
10 years, 7 months ago (2013-08-08 18:03:46 UTC) #2
volker.dobler
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#newcode71 src/pkg/net/dnsname_test.go:71: s16 := "0123456789abcdef" On 2013/08/08 18:03:46, bradfitz wrote: ...
10 years, 7 months ago (2013-08-08 22:08:08 UTC) #3
bradfitz
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 src/pkg/net/dnsname_test.go:79: b.Fatalf("isDomainName(%q)", tc.name) Errorf, not Fatalf. Also: "isDomainName(%q) = %v; ...
10 years, 7 months ago (2013-08-08 22:23:26 UTC) #4
volker.dobler
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#newcode79 src/pkg/net/dnsname_test.go:79: b.Fatalf("isDomainName(%q)", tc.name) On 2013/08/08 22:23:26, bradfitz wrote: > ...
10 years, 7 months ago (2013-08-08 23:21:40 UTC) #5
bradfitz
Let's say I make a change. I break something. I'd rather see the 7 cases ...
10 years, 7 months ago (2013-08-08 23:29:13 UTC) #6
bradfitz
LGTM On Thu, Aug 8, 2013 at 4:29 PM, Brad Fitzpatrick <bradfitz@golang.org>wrote: > Let's say ...
10 years, 7 months ago (2013-08-08 23:33:49 UTC) #7
bradfitz
10 years, 7 months ago (2013-08-08 23:33:59 UTC) #8
*** 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>
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b