Issue 110100043: code review 110100043: strings: remove byteBitmap (Closed)
Descriptionstrings: remove byteBitmap
Previously we had a bitmap to check whether or not a byte
appears in a string should be replaced. But we don't actually
need a separate bitmap for that purpose. Removing the bitmap
makes the code simpler.
Patch Set 1 #Patch Set 2 : diff -r c19c9a063fe8 https://code.google.com/p/go #Patch Set 3 : diff -r c19c9a063fe8 https://code.google.com/p/go #Patch Set 4 : code review 110100043: strings: remove byteBitmap #Patch Set 5 : diff -r d628e9e34434 https://code.google.com/p/go #
Total comments: 2
Patch Set 6 : diff -r 41a383d40558 https://code.google.com/p/go #Patch Set 7 : diff -r 2c57aaea79c4 https://code.google.com/p/go #Patch Set 8 : diff -r 2c57aaea79c4 https://code.google.com/p/go #
Total comments: 11
Patch Set 9 : diff -r df1343c36dd5 https://code.google.com/p/go #Patch Set 10 : diff -r df1343c36dd5 https://code.google.com/p/go #MessagesTotal messages: 20
Hello golang-codereviews@googlegroups.com, I'd like you to review this change to https://code.google.com/p/go
Sign in to reply to this message.
Ping. In the other review thread an innocent-looking refactoring CL was found that it would make optimized code slower, so I ran the benchmark for this CL just in case. It actually looks good. Optimization is not the aim of this CL, though. I'm just trying to simplify. benchmark old ns/op new ns/op delta BenchmarkGenericNoMatch 779 809 +3.85% BenchmarkGenericMatch1 7696 7685 -0.14% BenchmarkGenericMatch2 39606 38531 -2.71% BenchmarkSingleMaxSkipping 1796 1797 +0.06% BenchmarkSingleLongSuffixFail 1427 1429 +0.14% BenchmarkSingleMatch 54735 53826 -1.66% BenchmarkByteByteNoMatch 512 238 -53.52% BenchmarkByteByteMatch 1096 597 -45.53% BenchmarkByteStringMatch 1634 1202 -26.44% BenchmarkHTMLEscapeNew 421 359 -14.73% BenchmarkHTMLEscapeOld 645 655 +1.55% BenchmarkByteStringReplacerWriteString 19463 15915 -18.23% BenchmarkByteReplacerWriteString 3621 3594 -0.75% BenchmarkByteByteReplaces 7571 7502 -0.91% BenchmarkByteByteMap 2532 2527 -0.20% BenchmarkIndexRune 78 80 +1.78% BenchmarkIndexRuneFastPath 27 27 +0.00% BenchmarkIndex 28 28 -0.35% BenchmarkIndexByte 22 22 +0.00% BenchmarkMapNoChanges 270 272 +0.74% BenchmarkIndexHard1 2476338 2474836 -0.06% BenchmarkIndexHard2 2478723 2478729 +0.00% BenchmarkIndexHard3 2477274 2477937 +0.03% BenchmarkCountHard1 2497697 2470645 -1.08% BenchmarkCountHard2 2464890 2462890 -0.08% BenchmarkCountHard3 2463013 2460426 -0.11% BenchmarkIndexTorture 17546 17527 -0.11% BenchmarkCountTorture 17596 17571 -0.14% BenchmarkCountTortureOverlapping 7505680 7500039 -0.08% BenchmarkFields 23374117 23348591 -0.11% BenchmarkFieldsFunc 23385385 23329651 -0.24% BenchmarkSplit1 19109965 18316018 -4.15% BenchmarkSplit2 4981009 5260505 +5.61% BenchmarkSplit3 5051219 5065164 +0.28% BenchmarkRepeat 185 183 -1.08%
Sign in to reply to this message.
https://codereview.appspot.com/110100043/diff/70001/src/pkg/strings/replace.go File src/pkg/strings/replace.go (right): https://codereview.appspot.com/110100043/diff/70001/src/pkg/strings/replace.g... src/pkg/strings/replace.go:467: if r[b] != nil { what about if l := len(r[b]) ; l > 0 { anyChanges = true newSize += l } else { newSize++ }
Sign in to reply to this message.
Hello golang-codereviews@googlegroups.com, dave@cheney.net (cc: golang-codereviews@googlegroups.com), Please take another look.
Sign in to reply to this message.
https://codereview.appspot.com/110100043/diff/70001/src/pkg/strings/replace.go File src/pkg/strings/replace.go (right): https://codereview.appspot.com/110100043/diff/70001/src/pkg/strings/replace.g... src/pkg/strings/replace.go:467: if r[b] != nil { Good suggestion. I found we can make it even better.
Sign in to reply to this message.
On 2014/06/25 06:45:29, ruiu wrote: > https://codereview.appspot.com/110100043/diff/70001/src/pkg/strings/replace.go > File src/pkg/strings/replace.go (right): > > https://codereview.appspot.com/110100043/diff/70001/src/pkg/strings/replace.g... > src/pkg/strings/replace.go:467: if r[b] != nil { > Good suggestion. I found we can make it even better. Comparing the slice to nil feels icky to me, when really you're saying "how many values are in this slice". I couldn't see how to do the same for the second occurrence of r[b] == nil in that method.
Sign in to reply to this message.
On 2014/06/25 06:47:59, dfc wrote: > On 2014/06/25 06:45:29, ruiu wrote: > > https://codereview.appspot.com/110100043/diff/70001/src/pkg/strings/replace.go > > File src/pkg/strings/replace.go (right): > > > > > https://codereview.appspot.com/110100043/diff/70001/src/pkg/strings/replace.g... > > src/pkg/strings/replace.go:467: if r[b] != nil { > > Good suggestion. I found we can make it even better. > > Comparing the slice to nil feels icky to me, when really you're saying "how many > values are in this slice". I couldn't see how to do the same for the second > occurrence of r[b] == nil in that method. So this and the second place what I'm saying now is not "how many values are in the slice" but "does this have a valid value?" It doesn't feel icky to me but does that mean my taste is not trained enough for Go yet?
Sign in to reply to this message.
Ping. One way to avoid comparing against nil slice would be to change the type of byteStringReplacer from [256][]byte to [256]*[]byte. But it feels to me that such change does't make much sense, as I'm fine with the original type and see no benefit of changing it. What do you think?
Sign in to reply to this message.
On 2014/06/30 21:59:44, ruiu wrote: > Ping. > > One way to avoid comparing against nil slice would be to change the type of > byteStringReplacer from [256][]byte to [256]*[]byte. But it feels to me that > such change does't make much sense, as I'm fine with the original type and see > no benefit of changing it. What do you think? panda(~/go/src/pkg/strings) % hg clpatch 110100043 abort: codereview issue 110100043 is out of date: patch and recent changes conflict (41a383d40558->2c57aaea79c4) Could you please remail this patch.
Sign in to reply to this message.
R=nigeltao@golang.org (assigned by dave@cheney.net)
Sign in to reply to this message.
Hello golang-codereviews@googlegroups.com, dave@cheney.net, gobot@golang.org, nigeltao@golang.org (cc: golang-codereviews@googlegroups.com), Please take another look.
Sign in to reply to this message.
On 2014/07/01 05:39:31, ruiu wrote: > Hello mailto:golang-codereviews@googlegroups.com, mailto:dave@cheney.net, mailto:gobot@golang.org, > mailto:nigeltao@golang.org (cc: mailto:golang-codereviews@googlegroups.com), > > Please take another look. LGTM. I'm happy with this change, but I'd like to see another LGTM before you land it.
Sign in to reply to this message.
LGTM
Sign in to reply to this message.
https://codereview.appspot.com/110100043/diff/130001/src/pkg/strings/replace.go File src/pkg/strings/replace.go (left): https://codereview.appspot.com/110100043/diff/130001/src/pkg/strings/replace.... src/pkg/strings/replace.go:499: newSize++ this seems wrong... why is this increment going away for bytes not being replaced? https://codereview.appspot.com/110100043/diff/130001/src/pkg/strings/replace.go File src/pkg/strings/replace.go (right): https://codereview.appspot.com/110100043/diff/130001/src/pkg/strings/replace.... src/pkg/strings/replace.go:43: r := byteReplacer{} personal nit: I like to use value types locally only when they don't escape. If you're going to return &r down below later, I'd just write r := new(byteReplacer) here instead, which hints to the reader that it's being returned. at least to me. https://codereview.appspot.com/110100043/diff/130001/src/pkg/strings/replace.... src/pkg/strings/replace.go:57: r := byteStringReplacer{} likewise
Sign in to reply to this message.
ignore; testing
Sign in to reply to this message.
https://codereview.appspot.com/110100043/diff/130001/src/pkg/strings/replace.go File src/pkg/strings/replace.go (left): https://codereview.appspot.com/110100043/diff/130001/src/pkg/strings/replace.... src/pkg/strings/replace.go:499: newSize++ See line 491 (or line 464 in the new file). https://codereview.appspot.com/110100043/diff/130001/src/pkg/strings/replace.go File src/pkg/strings/replace.go (right): https://codereview.appspot.com/110100043/diff/130001/src/pkg/strings/replace.... src/pkg/strings/replace.go:43: r := byteReplacer{} I chose this because Dave prefers this style (https://codereview.appspot.com/102550043/diff/60001/src/pkg/strings/replace.g...). I don't have a strong preference on this.
Sign in to reply to this message.
Ping.
Sign in to reply to this message.
LGTM. https://codereview.appspot.com/110100043/diff/130001/src/pkg/strings/replace.go File src/pkg/strings/replace.go (right): https://codereview.appspot.com/110100043/diff/130001/src/pkg/strings/replace.... src/pkg/strings/replace.go:458: // The array contain replacement byte slices indexed by old byte. s/contain/contains/ https://codereview.appspot.com/110100043/diff/130001/src/pkg/strings/replace.... src/pkg/strings/replace.go:459: // A replacement byte slice may be nil if the old byte of the index I'd s/may/will/, or just say: // A nil []byte means that the old byte should not be replaced. https://codereview.appspot.com/110100043/diff/130001/src/pkg/strings/replace.... src/pkg/strings/replace.go:470: newSize += len(r[b]) - 1 The "-1" probably needs a comment: // The -1 is because we are replacing 1 byte with len(r[b]) bytes. newSize += len(r[b]) - 1
Sign in to reply to this message.
https://codereview.appspot.com/110100043/diff/130001/src/pkg/strings/replace.go File src/pkg/strings/replace.go (right): https://codereview.appspot.com/110100043/diff/130001/src/pkg/strings/replace.... src/pkg/strings/replace.go:458: // The array contain replacement byte slices indexed by old byte. On 2014/07/17 06:31:56, nigeltao wrote: > s/contain/contains/ Done. https://codereview.appspot.com/110100043/diff/130001/src/pkg/strings/replace.... src/pkg/strings/replace.go:459: // A replacement byte slice may be nil if the old byte of the index On 2014/07/17 06:31:56, nigeltao wrote: > I'd s/may/will/, or just say: > > // A nil []byte means that the old byte should not be replaced. Done. https://codereview.appspot.com/110100043/diff/130001/src/pkg/strings/replace.... src/pkg/strings/replace.go:470: newSize += len(r[b]) - 1 On 2014/07/17 06:31:56, nigeltao wrote: > The "-1" probably needs a comment: > > // The -1 is because we are replacing 1 byte with len(r[b]) bytes. > newSize += len(r[b]) - 1 Done.
Sign in to reply to this message.
*** Submitted as https://code.google.com/p/go/source/detail?r=b850cb862bb2 *** strings: remove byteBitmap Previously we had a bitmap to check whether or not a byte appears in a string should be replaced. But we don't actually need a separate bitmap for that purpose. Removing the bitmap makes the code simpler. LGTM=dave, iant, nigeltao R=golang-codereviews, dave, gobot, nigeltao, iant, bradfitz, rsc CC=golang-codereviews https://codereview.appspot.com/110100043
Sign in to reply to this message.
|
