strings: fix NewReplacer(old0, new0, old1, new1, ...) to be consistent

when oldi == oldj.

Benchmark numbers show no substantial change.

R=eric.d.eisner, rogpeppe
CC=golang-dev
https://golang.org/cl/6496104

diff --git a/src/pkg/strings/replace.go b/src/pkg/strings/replace.go
index f53a96ee0f9c4c74069150d21ac7884a20ee571d..3a1322badeb066b12245ad5f2d76c2d8b295b610 100644 (file)
--- a/src/pkg/strings/replace.go
+++ b/src/pkg/strings/replace.go
@@ -33,47 +33,41 @@ func NewReplacer(oldnew ...string) *Replacer {
                panic("strings.NewReplacer: odd argument count")
        }
 
-       // Possible implementations.
-       var (
-               bb  byteReplacer
-               bs  byteStringReplacer
-               gen genericReplacer
-       )
-
-       allOldBytes, allNewBytes := true, true
-       for len(oldnew) > 0 {
-               old, new := oldnew[0], oldnew[1]
-               oldnew = oldnew[2:]
-               if len(old) != 1 {
-                       allOldBytes = false
+       allNewBytes := true
+       for i := 0; i < len(oldnew); i += 2 {
+               if len(oldnew[i]) != 1 {
+                       return &Replacer{r: makeGenericReplacer(oldnew)}
                }
-               if len(new) != 1 {
+               if len(oldnew[i+1]) != 1 {
                        allNewBytes = false
                }
+       }
 
-               // generic
-               gen.p = append(gen.p, pair{old, new})
-
-               // byte -> string
-               if allOldBytes {
-                       bs.old.set(old[0])
-                       bs.new[old[0]] = []byte(new)
-               }
-
-               // byte -> byte
-               if allOldBytes && allNewBytes {
-                       bb.old.set(old[0])
-                       bb.new[old[0]] = new[0]
+       if allNewBytes {
+               bb := &byteReplacer{}
+               for i := 0; i < len(oldnew); i += 2 {
+                       o, n := oldnew[i][0], oldnew[i+1][0]
+                       if bb.old[o>>5]&uint32(1<<(o&31)) != 0 {
+                               // Later old->new maps do not override previous ones with the same old string.
+                               continue
+                       }
+                       bb.old.set(o)
+                       bb.new[o] = n
                }
+               return &Replacer{r: bb}
        }
 
-       if allOldBytes && allNewBytes {
-               return &Replacer{r: &bb}
-       }
-       if allOldBytes {
-               return &Replacer{r: &bs}
+       bs := &byteStringReplacer{}
+       for i := 0; i < len(oldnew); i += 2 {
+               o, new := oldnew[i][0], oldnew[i+1]
+               if bs.old[o>>5]&uint32(1<<(o&31)) != 0 {
+                       // Later old->new maps do not override previous ones with the same old string.
+                       continue
+               }
+               bs.old.set(o)
+               bs.new[o] = []byte(new)
        }
-       return &Replacer{r: &gen}
+       return &Replacer{r: bs}
 }
 
 // Replace returns a copy of s with all replacements performed.
@@ -94,6 +88,16 @@ type genericReplacer struct {
 
 type pair struct{ old, new string }
 
+func makeGenericReplacer(oldnew []string) *genericReplacer {
+       gen := &genericReplacer{
+               p: make([]pair, len(oldnew)/2),
+       }
+       for i := 0; i < len(oldnew); i += 2 {
+               gen.p[i/2] = pair{oldnew[i], oldnew[i+1]}
+       }
+       return gen
+}
+
 type appendSliceWriter struct {
        b []byte
 }

diff --git a/src/pkg/strings/replace_test.go b/src/pkg/strings/replace_test.go
index 0b01d3674fca15b818e02a51203a7f5952691fe9..7a960986bbe06a5ab8d0f36ab1c6ebaf9b96afa8 100644 (file)
--- a/src/pkg/strings/replace_test.go
+++ b/src/pkg/strings/replace_test.go
@@ -70,7 +70,7 @@ func TestReplacer(t *testing.T) {
                testCase{inc, "\x00\xff", "\x01\x00"},
                testCase{inc, "", ""},
 
-               testCase{NewReplacer("a", "1", "a", "2"), "brad", "br2d"}, // TODO: should this be "br1d"?
+               testCase{NewReplacer("a", "1", "a", "2"), "brad", "br1d"},
        )
 
        // repeat maps "a"->"a", "b"->"bb", "c"->"ccc", ...
@@ -95,7 +95,7 @@ func TestReplacer(t *testing.T) {
                testCase{repeat, "abba", "abbbba"},
                testCase{repeat, "", ""},
 
-               testCase{NewReplacer("a", "11", "a", "22"), "brad", "br22d"}, // TODO: should this be "br11d"?
+               testCase{NewReplacer("a", "11", "a", "22"), "brad", "br11d"},
        )
 
        // The remaining test cases have variable length old strings.
@@ -246,6 +246,14 @@ func TestReplacer(t *testing.T) {
                testCase{blankFoo, "", "X"},
        )
 
+       // No-arg test cases.
+
+       nop := NewReplacer()
+       testCases = append(testCases,
+               testCase{nop, "abc", "abc"},
+               testCase{nop, "", ""},
+       )
+
        // Run the test cases.
 
        for i, tc := range testCases {
@@ -277,9 +285,10 @@ func TestPickAlgorithm(t *testing.T) {
                want string
        }{
                {capitalLetters, "*strings.byteReplacer"},
+               {htmlEscaper, "*strings.byteStringReplacer"},
                {NewReplacer("12", "123"), "*strings.genericReplacer"},
                {NewReplacer("1", "12"), "*strings.byteStringReplacer"},
-               {htmlEscaper, "*strings.byteStringReplacer"},
+               {NewReplacer("a", "1", "b", "12", "cde", "123"), "*strings.genericReplacer"},
        }
        for i, tc := range testCases {
                got := fmt.Sprintf("%T", tc.r.Replacer())