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

Issue 110100043: code review 110100043: strings: remove byteBitmap (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
10 years, 8 months ago by ruiu
Modified:
10 years, 8 months ago
Reviewers:
nigeltao, dave, iant
CC:
golang-codereviews, dave_cheney.net, gobot, nigeltao, iant, bradfitz, rsc
Visibility:
Public.

Description

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.

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 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+35 lines, -65 lines) Patch
M src/pkg/strings/replace.go View 1 2 3 4 5 6 7 8 8 chunks +35 lines, -65 lines 0 comments Download

Messages

Total messages: 20
ruiu
Hello golang-codereviews@googlegroups.com, I'd like you to review this change to https://code.google.com/p/go
10 years, 8 months ago (2014-06-22 05:39:58 UTC) #1
ruiu
Ping. In the other review thread an innocent-looking refactoring CL was found that it would ...
10 years, 8 months ago (2014-06-25 06:34:28 UTC) #2
dave_cheney.net
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.go#newcode467 src/pkg/strings/replace.go:467: if r[b] != nil { what about if l ...
10 years, 8 months ago (2014-06-25 06:42:58 UTC) #3
ruiu
Hello golang-codereviews@googlegroups.com, dave@cheney.net (cc: golang-codereviews@googlegroups.com), Please take another look.
10 years, 8 months ago (2014-06-25 06:45:19 UTC) #4
ruiu
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.go#newcode467 src/pkg/strings/replace.go:467: if r[b] != nil { Good suggestion. I found ...
10 years, 8 months ago (2014-06-25 06:45:29 UTC) #5
dave_cheney.net
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.go#newcode467 > ...
10 years, 8 months ago (2014-06-25 06:47:59 UTC) #6
ruiu
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 > ...
10 years, 8 months ago (2014-06-25 06:55:07 UTC) #7
ruiu
Ping. One way to avoid comparing against nil slice would be to change the type ...
10 years, 8 months ago (2014-06-30 21:59:44 UTC) #8
dave_cheney.net
On 2014/06/30 21:59:44, ruiu wrote: > Ping. > > One way to avoid comparing against ...
10 years, 8 months ago (2014-07-01 05:25:15 UTC) #9
gobot
R=nigeltao@golang.org (assigned by dave@cheney.net)
10 years, 8 months ago (2014-07-01 05:31:03 UTC) #10
ruiu
Hello golang-codereviews@googlegroups.com, dave@cheney.net, gobot@golang.org, nigeltao@golang.org (cc: golang-codereviews@googlegroups.com), Please take another look.
10 years, 8 months ago (2014-07-01 05:39:31 UTC) #11
dave_cheney.net
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), > ...
10 years, 8 months ago (2014-07-01 05:44:45 UTC) #12
iant
LGTM
10 years, 8 months ago (2014-07-01 13:35:53 UTC) #13
bradfitz
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.go#oldcode499 src/pkg/strings/replace.go:499: newSize++ this seems wrong... why is this increment going ...
10 years, 8 months ago (2014-07-01 16:12:26 UTC) #14
rsc
ignore; testing
10 years, 8 months ago (2014-07-01 18:01:20 UTC) #15
ruiu
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.go#oldcode499 src/pkg/strings/replace.go:499: newSize++ See line 491 (or line 464 in the ...
10 years, 8 months ago (2014-07-01 18:04:52 UTC) #16
ruiu
Ping.
10 years, 8 months ago (2014-07-16 21:21:38 UTC) #17
nigeltao
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.go#newcode458 src/pkg/strings/replace.go:458: // The array contain replacement byte slices indexed ...
10 years, 8 months ago (2014-07-17 06:31:56 UTC) #18
ruiu
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.go#newcode458 src/pkg/strings/replace.go:458: // The array contain replacement byte slices indexed by ...
10 years, 8 months ago (2014-07-17 16:50:44 UTC) #19
ruiu
10 years, 8 months ago (2014-07-17 16:55:15 UTC) #20
*** 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.

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