code review 247041: regexp: add ReplaceAllFunc and ReplaceAllStringFunc

regexp: add ReplaceAllFunc and ReplaceAllStringFunc

[adg, 2010-03-05 05:15:35]

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

I'd like you to review this change.

[adg, 2010-03-05 05:19:30]

There are probably performance issues with this, unless the compiler
is clever enough to inline it.

I've already got a use-case for these new functions. They're *very* handy.

On 5 March 2010 16:15,  <adg@golang.org> wrote:
> Reviewers: r,
>
> Message:
> Hello r (cc: golang-dev@googlegroups.com),
>
> I'd like you to review this change.
>
>
> Description:
> regexp: add ReplaceAllFunc and ReplaceAllStringFunc
>
> Please review this at http://codereview.appspot.com/247041/show
>
> Affected files:
>  M src/pkg/regexp/regexp.go
>
>
> Index: src/pkg/regexp/regexp.go
> ===================================================================
> --- a/src/pkg/regexp/regexp.go
> +++ b/src/pkg/regexp/regexp.go
> @@ -1006,6 +1006,14 @@
>  // have been replaced by repl.  No support is provided for expressions
>  // (e.g. \1 or $1) in the replacement string.
>  func (re *Regexp) ReplaceAllString(src, repl string) string {
> +       return re.ReplaceAllStringFunc(src, func(string) string { return
> repl })
> +}
> +
> +// ReplaceAllStringFunc returns a copy of src in which all matches for the
> +// Regexp have been replaced by the return value of of function repl (whose
> +// first argument is the matched string).  No support is provided for
> +// expressions (e.g. \1 or $1) in the replacement string.
> +func (re *Regexp) ReplaceAllStringFunc(src string, repl func(string)
> string) string {
>        lastMatchEnd := 0 // end position of the most recent match
>        searchPos := 0    // position where we next look for a match
>        buf := new(bytes.Buffer)
> @@ -1023,7 +1031,7 @@
>                // (Otherwise, we get double replacement for patterns that
>                // match both empty and nonempty strings.)
>                if a[1] > lastMatchEnd || a[0] == 0 {
> -                       io.WriteString(buf, repl)
> +                       io.WriteString(buf, repl(src[a[0]:a[1]]))
>                }
>                lastMatchEnd = a[1]
>
> @@ -1050,6 +1058,14 @@
>  // have been replaced by repl.  No support is provided for expressions
>  // (e.g. \1 or $1) in the replacement text.
>  func (re *Regexp) ReplaceAll(src, repl []byte) []byte {
> +       return re.ReplaceAllFunc(src, func([]byte) []byte { return repl })
> +}
> +
> +// ReplaceAllFunc returns a copy of src in which all matches for the
> +// Regexp have been replaced by the return value of of function repl (whose
> +// first argument is the matched []byte).  No support is provided for
> +// expressions (e.g. \1 or $1) in the replacement string.
> +func (re *Regexp) ReplaceAllFunc(src []byte, repl func([]byte) []byte)
> []byte {
>        lastMatchEnd := 0 // end position of the most recent match
>        searchPos := 0    // position where we next look for a match
>        buf := new(bytes.Buffer)
> @@ -1067,7 +1083,7 @@
>                // (Otherwise, we get double replacement for patterns that
>                // match both empty and nonempty strings.)
>                if a[1] > lastMatchEnd || a[0] == 0 {
> -                       buf.Write(repl)
> +                       buf.Write(repl(src[a[0]:a[1]]))
>                }
>                lastMatchEnd = a[1]
>
>
>
>

[r2, 2010-03-05 07:14:52]

I'm not against it in principle - I like functions.  A benchmark to see if the
existing routine is slowed down significantly would be nice (I doubt it,
actually).  And you certainly need tests.

-rob

On Mar 4, 2010, at 9:19 PM, Andrew Gerrand wrote:

> There are probably performance issues with this, unless the compiler
> is clever enough to inline it.
> 
> I've already got a use-case for these new functions. They're *very* handy.
> 
> On 5 March 2010 16:15,  <adg@golang.org> wrote:
>> Reviewers: r,
>> 
>> Message:
>> Hello r (cc: golang-dev@googlegroups.com),
>> 
>> I'd like you to review this change.
>> 
>> 
>> Description:
>> regexp: add ReplaceAllFunc and ReplaceAllStringFunc
>> 
>> Please review this at http://codereview.appspot.com/247041/show
>> 
>> Affected files:
>>  M src/pkg/regexp/regexp.go
>> 
>> 
>> Index: src/pkg/regexp/regexp.go
>> ===================================================================
>> --- a/src/pkg/regexp/regexp.go
>> +++ b/src/pkg/regexp/regexp.go
>> @@ -1006,6 +1006,14 @@
>>  // have been replaced by repl.  No support is provided for expressions
>>  // (e.g. \1 or $1) in the replacement string.
>>  func (re *Regexp) ReplaceAllString(src, repl string) string {
>> +       return re.ReplaceAllStringFunc(src, func(string) string { return
>> repl })
>> +}
>> +
>> +// ReplaceAllStringFunc returns a copy of src in which all matches for the
>> +// Regexp have been replaced by the return value of of function repl (whose
>> +// first argument is the matched string).  No support is provided for
>> +// expressions (e.g. \1 or $1) in the replacement string.
>> +func (re *Regexp) ReplaceAllStringFunc(src string, repl func(string)
>> string) string {
>>        lastMatchEnd := 0 // end position of the most recent match
>>        searchPos := 0    // position where we next look for a match
>>        buf := new(bytes.Buffer)
>> @@ -1023,7 +1031,7 @@
>>                // (Otherwise, we get double replacement for patterns that
>>                // match both empty and nonempty strings.)
>>                if a[1] > lastMatchEnd || a[0] == 0 {
>> -                       io.WriteString(buf, repl)
>> +                       io.WriteString(buf, repl(src[a[0]:a[1]]))
>>                }
>>                lastMatchEnd = a[1]
>> 
>> @@ -1050,6 +1058,14 @@
>>  // have been replaced by repl.  No support is provided for expressions
>>  // (e.g. \1 or $1) in the replacement text.
>>  func (re *Regexp) ReplaceAll(src, repl []byte) []byte {
>> +       return re.ReplaceAllFunc(src, func([]byte) []byte { return repl })
>> +}
>> +
>> +// ReplaceAllFunc returns a copy of src in which all matches for the
>> +// Regexp have been replaced by the return value of of function repl (whose
>> +// first argument is the matched []byte).  No support is provided for
>> +// expressions (e.g. \1 or $1) in the replacement string.
>> +func (re *Regexp) ReplaceAllFunc(src []byte, repl func([]byte) []byte)
>> []byte {
>>        lastMatchEnd := 0 // end position of the most recent match
>>        searchPos := 0    // position where we next look for a match
>>        buf := new(bytes.Buffer)
>> @@ -1067,7 +1083,7 @@
>>                // (Otherwise, we get double replacement for patterns that
>>                // match both empty and nonempty strings.)
>>                if a[1] > lastMatchEnd || a[0] == 0 {
>> -                       buf.Write(repl)
>> +                       buf.Write(repl(src[a[0]:a[1]]))
>>                }
>>                lastMatchEnd = a[1]
>> 
>> 
>> 
>> 

[adg, 2010-03-06 22:43:16]

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

I'd like you to review this change.

[adg, 2010-03-06 22:49:12]

I've added some tests. I'm open to suggestions as to more test cases
(most test cases are already covered by the ReplaceAll tests).

I added benchmarks. The new method seems to be about 2-3% slower:

regexp.BenchmarkReplaceAll	  100000	     22541 ns/op
regexp.BenchmarkOldReplaceAll	  100000	     22083 ns/op

regexp.BenchmarkReplaceAll	  100000	     22381 ns/op
regexp.BenchmarkOldReplaceAll	  100000	     21749 ns/op

I wasn't sure of the best way to include the original function for
benchmarking. I called it oldReplaceAllString and put it in
all_tests.go. Should I remove this before the final submit?

Andrew

On 7 March 2010 09:43,  <adg@golang.org> wrote:
> Hello r (cc: golang-dev@googlegroups.com),
>
> I'd like you to review this change.
>
>
> http://codereview.appspot.com/247041/show
>

[r2, 2010-03-06 23:59:40]

On Mar 6, 2010, at 2:48 PM, Andrew Gerrand wrote:

> I've added some tests. I'm open to suggestions as to more test cases
> (most test cases are already covered by the ReplaceAll tests).
> 
> I added benchmarks. The new method seems to be about 2-3% slower:

As I suspected - not an issue at all.

> regexp.BenchmarkReplaceAll	  100000	     22541 ns/op
> regexp.BenchmarkOldReplaceAll	  100000	     22083 ns/op
> 
> regexp.BenchmarkReplaceAll	  100000	     22381 ns/op
> regexp.BenchmarkOldReplaceAll	  100000	     21749 ns/op
> 
> I wasn't sure of the best way to include the original function for
> benchmarking. I called it oldReplaceAllString and put it in
> all_tests.go. Should I remove this before the final submit?

Yes, it's only there to check that you hadn't made things much slower. 

-rob

[r, 2010-03-07 00:02:09]

LGTM

but please delete dregs.

this is really nice.

http://codereview.appspot.com/247041/diff/11001/12001
File src/pkg/regexp/all_test.go (right):

http://codereview.appspot.com/247041/diff/11001/12001#newcode560
src/pkg/regexp/all_test.go:560: func BenchmarkOldReplaceAll(b *testing.B) {
you can delete from here to the end.

[adg, 2010-03-07 01:41:55]

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

regexp: add ReplaceAllFunc and ReplaceAllStringFunc

R=r
CC=golang-dev
http://codereview.appspot.com/247041

Committer: Andrew Gerrand <adg@golang.org>