code review 1731048: bytes, strings: add Replace

bytes, strings: add Replace

This is the Replace I suggested in the review of CL 1114041.
It's true that we already have

	regexp.MustCompile(regexp.QuoteMeta(old)).ReplaceAll(s, new)

but because this Replace is doing a simpler job it is
simpler to call and inherently more efficient.

I will add the bytes implementation and tests to the
CL after the strings one has been reviewed.

[rsc, 2010-06-30 21:41:15]

Hello r, cw (cc: golang-dev@googlegroups.com),

I'd like you to review this change.

[r, 2010-06-30 21:55:19]

http://codereview.appspot.com/1731048/diff/2001/3001
File src/pkg/strings/strings.go (right):

http://codereview.appspot.com/1731048/diff/2001/3001#newcode467
src/pkg/strings/strings.go:467: if old == new {
is this necessary? it's a rare case. worth checking? i think the code handles it
already

http://codereview.appspot.com/1731048/diff/2001/3001#newcode478
src/pkg/strings/strings.go:478: }
get rid of m
 if m := Count(s, old); m == 0 {
   return s
 } else if m < n {
   n = m
 }

and use n below.

http://codereview.appspot.com/1731048/diff/2001/3001#newcode495
src/pkg/strings/strings.go:495: w += copyString(t[w:], new)
this overwrites. looks wrong, or at least inefficient

[rsc, 2010-06-30 22:22:27]

> is this necessary? it's a rare case. worth checking? i think the code
> handles it already

true; it avoids an allocation though.
i put that in a comment.

> http://codereview.appspot.com/1731048/diff/2001/3001#newcode478
> src/pkg/strings/strings.go:478: }
> get rid of m
>  if m := Count(s, old); m == 0 {
>   return s
>  } else if m < n {
>   n = m
>  }
>
> and use n below.

done

> http://codereview.appspot.com/1731048/diff/2001/3001#newcode495
> src/pkg/strings/strings.go:495: w += copyString(t[w:], new)
> this overwrites. looks wrong, or at least inefficient

i don't understand.  copyString is copy but takes a string as the
second argument.

[rsc, 2010-06-30 22:22:39]

Hello r, cw (cc: golang-dev@googlegroups.com),

Please take another look.

[r2, 2010-06-30 22:29:29]

LGTM

On Jun 30, 2010, at 3:22 PM, Russ Cox wrote:

>> is this necessary? it's a rare case. worth checking? i think the code
>> handles it already
>
> true; it avoids an allocation though.
> i put that in a comment.
>
>> http://codereview.appspot.com/1731048/diff/2001/3001#newcode478
>> src/pkg/strings/strings.go:478: }
>> get rid of m
>>  if m := Count(s, old); m == 0 {
>>   return s
>>  } else if m < n {
>>   n = m
>>  }
>>
>> and use n below.
>
> done
>
>> http://codereview.appspot.com/1731048/diff/2001/3001#newcode495
>> src/pkg/strings/strings.go:495: w += copyString(t[w:], new)
>> this overwrites. looks wrong, or at least inefficient
>
> i don't understand.  copyString is copy but takes a string as the
> second argument.

[cw, 2010-06-30 22:35:56]

I always wondered why we don't have a bytes version of these functions then use
that as a basis for strings?  Is this to avoid the dependency?

We could also consider ugly unsafe optimizations to avoid the copies from
bytes->string when we know the bytes version will never be references outside
and thus should be safe to be considered immuatable (though this would fail late
on if we allocted strings differently and enforce RO access).

[rsc, 2010-06-30 23:36:42]

On Wed, Jun 30, 2010 at 15:35,  <cw@f00f.org> wrote:
> I always wondered why we don't have a bytes version of these functions
> then use that as a basis for strings?  Is this to avoid the dependency?

It's to avoid the copy that a conversion would entail.
string -> bytes is a copy too.

Russ

[cw, 2010-07-01 00:10:36]

On Wed, Jun 30, 2010 at 04:36:36PM -0700, Russ Cox wrote:

> It's to avoid the copy that a conversion would entail.
> string -> bytes is a copy too.

sure, but for strings.Replace we have to copy no matter what, the
input must remain unchanged

so why not copy once to []byte and use unsafe to create a string from
that avoiding the second copy from []byte -> string?

[rsc, 2010-07-01 00:20:12]

> so why not copy once to []byte and use unsafe to create a string from
> that avoiding the second copy from []byte -> string?

it's very nice that the tree is, on the whole,
free from imports of unsafe.  if something as
simple as the strings package is importing unsafe,
something is very wrong.  we might as well go
back to programming in c.

russ

[rsc, 2010-07-01 00:41:42]

Please take another look.

Added bytes routines to CL.

[r, 2010-07-01 00:45:20]

LGTM

my only concern is whether n==0 should mean forever, or only n < 0. i can see a
program correctly using n==0 and expecting no change.

[rsc, 2010-07-01 00:48:56]

On Wed, Jun 30, 2010 at 17:45,  <r@golang.org> wrote:
> LGTM
>
> my only concern is whether n==0 should mean forever, or only n < 0. i
> can see a program correctly using n==0 and expecting no change.

i wondered about that; what's there now is consistent with
the meaning established by Split.

[r2, 2010-07-01 00:50:26]

On Jun 30, 2010, at 5:48 PM, Russ Cox wrote:

> On Wed, Jun 30, 2010 at 17:45,  <r@golang.org> wrote:
>> LGTM
>>
>> my only concern is whether n==0 should mean forever, or only n < 0. i
>> can see a program correctly using n==0 and expecting no change.
>
> i wondered about that; what's there now is consistent with
> the meaning established by Split.

i know. i'm still not happy about it. maybe Split should change.

-rob

[rsc, 2010-07-01 01:03:13]

*** Submitted as http://code.google.com/p/go/source/detail?r=6470c731be0e ***

bytes, strings: add Replace

This is the Replace I suggested in the review of CL 1114041.
It's true that we already have

	regexp.MustCompile(regexp.QuoteMeta(old)).ReplaceAll(s, new)

but because this Replace is doing a simpler job it is
simpler to call and inherently more efficient.

I will add the bytes implementation and tests to the
CL after the strings one has been reviewed.

R=r, cw
CC=golang-dev
http://codereview.appspot.com/1731048