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

src/pkg/strings/replace.go

@@ -33,47 +33,41 @@ func NewReplacer(oldnew ...string) *Replacer {
 		panic("strings.NewReplacer: odd argument count")
 	}
 
-	// Possible implementations.
+	allNewBytes := true
-	var (
+	for i := 0; i < len(oldnew); i += 2 {
-		bb  byteReplacer
+		if len(oldnew[i]) != 1 {
-		bs  byteStringReplacer
+			return &Replacer{r: makeGenericReplacer(oldnew)}
-		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
 		}
-		if len(new) != 1 {
+		if len(oldnew[i+1]) != 1 {
 			allNewBytes = false
 		}
+	}
 
-		// generic
+	if allNewBytes {
-		gen.p = append(gen.p, pair{old, new})
+		bb := &byteReplacer{}
-
+		for i := 0; i < len(oldnew); i += 2 {
-		// byte -> string
+			o, n := oldnew[i][0], oldnew[i+1][0]
-		if allOldBytes {
+			if bb.old[o>>5]&uint32(1<<(o&31)) != 0 {
-			bs.old.set(old[0])
+				// Later old->new maps do not override previous ones with the same old string.
-			bs.new[old[0]] = []byte(new)
+				continue
+			}
+			bb.old.set(o)
+			bb.new[o] = n
 		}
+		return &Replacer{r: bb}
+	}
 
-		// byte -> byte
+	bs := &byteStringReplacer{}
-		if allOldBytes && allNewBytes {
+	for i := 0; i < len(oldnew); i += 2 {
-			bb.old.set(old[0])
+		o, new := oldnew[i][0], oldnew[i+1]
-			bb.new[old[0]] = new[0]
+		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: bs}
-	if allOldBytes && allNewBytes {
-		return &Replacer{r: &bb}
-	}
-	if allOldBytes {
-		return &Replacer{r: &bs}
-	}
-	return &Replacer{r: &gen}
 }
 
 // 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
 }

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())