Implement str.replace by S-YOU · Pull Request #168 · google/grumpy

S-YOU · 2017-01-19T17:11:00Z

Implementing str.replace.
This will have some updates from #167, so this is WIP for now.

trotterdylan

Thanks for taking this on! I didn't comment on the stuff relating to #167.

trotterdylan · 2017-01-19T17:13:51Z

+func strReplace(f *Frame, args Args, _ KWArgs) (*Object, *BaseException) {
+	// TODO: Support unicode replace.
+	expectedTypes := []*Type{StrType, StrType, StrType, ObjectType}
+	argc, n := len(args), -1


It's better not to group variables that are unrelated since it's harder to find their definition.

Thank you, make sense.

trotterdylan · 2017-01-19T17:14:23Z

+	}
+	s := toStrUnsafe(args[0]).Value()
+	old := toStrUnsafe(args[1]).Value()
+	new := toStrUnsafe(args[2]).Value()


Try to avoid using names that collide with builtin names.

Sure, too much copying from CPython.

trotterdylan · 2017-01-19T17:15:05Z

+	s := toStrUnsafe(args[0]).Value()
+	old := toStrUnsafe(args[1]).Value()
+	new := toStrUnsafe(args[2]).Value()
+	// CPython only, pypy supposed to be same as Go


Can you elaborate on this comment?

I haven't really tested on pypy, but looks like it is different.
http://bugs.python.org/issue28029
assert ''.replace('', 'x') == 'x'
assert ''.replace('', 'x', 1000) == ''

trotterdylan · 2017-01-19T17:19:17Z

+	if len(s) == 0 && len(old) == 0 && n >= 0 {
+		return NewStr("").ToObject(), nil
+	}
+	return NewStr(strings.Replace(s, old, new, n)).ToObject(), nil


I think strings.Replace() may not be compatible with the Python version because when old == "", strings.Replace: matches at the beginning of the string and after each UTF-8 sequence whereas in Python it will match after each byte. So if the string contains UTF-8 sequences, the results will differ (this is a good test case to have, as well).

Oh, I haven't think about UTF-8 yet, also unicode.

I am not very sure which utf8 sequence is best to test, but added some, tests still passed.

S-YOU · 2017-01-19T17:55:16Z

Rebased, fix reviewed issues
Added utf8 replace test cases, but not sure I am testing real issues.

trotterdylan · 2017-01-19T18:04:12Z

Thanks for the changes! The issue I'm pointing out is that in Go strings.Replace() treats the input as a UTF-8 encoded string and if the old string is "" then it will add a replacement in between each UTF-8 code point. E.g.:

package main

import (
	"fmt"
	"strings"
)

func main() {
	fmt.Println([]byte(strings.Replace("\xd0\xb2\xd0\xbe\xd0\xbb", "", "\x00", -1)))
}

outputs: [0 208 178 0 208 190 0 208 187 0]

vs. CPython

>>> [ord(c) for c in"\xd0\xb2\xd0\xbe\xd0\xbb".replace('', '\0')]
[0, 208, 0, 178, 0, 208, 0, 190, 0, 208, 0, 187, 0]

I don't think it's a good idea to use strings.Replace() because of these subtle incompatibilities.

S-YOU · 2017-01-19T18:10:03Z

Hmm, I see. Is that mean, need to implement by looping through bytes?

trotterdylan · 2017-01-19T18:26:55Z

Looks like there's a bytes replace: https://golang.org/pkg/bytes/#Replace

…

On Thu, Jan 19, 2017 at 10:10 AM YOU ***@***.***> wrote: Hmm, I see. Is that mean, need to implement by looping through bytes? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#168 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAHLe3PEOmLAF9BityX9EMQMf3q1X4gHks5rT6b8gaJpZM4LoXS1> .

S-YOU · 2017-01-19T18:28:45Z

Thank you but it mention same thing,
If old is empty, it matches at the beginning of the slice and after each UTF-8 sequence

The issue is only on empty old string? then I can just write a loop for that.

trotterdylan · 2017-01-19T18:35:57Z

It's probably worth looking at the go source code to make sure that's the only peculiarity. But yeah maybe that's the best approach.

…

On Thu, Jan 19, 2017 at 10:28 AM YOU ***@***.***> wrote: Thank you but it mention same thing, If old is empty, it matches at the beginning of the slice and after each UTF-8 sequence The issue is only on empty old string? then I can just write a loop for that. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#168 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAHLe77RhrmxRCh4SMFbkyldE2ZmU3Oiks5rT6tegaJpZM4LoXS1> .

S-YOU · 2017-01-20T01:57:56Z

Updated. I checked both strings.Replace and bytes.Replace source. It appears that they doing special checking on len(old) == 0.

	            if len(old) == 0 {
			if i > 0 {
				_, wid := utf8.DecodeRuneInString(s[start:])
				j += wid
			}

So I added a loop to copy manually for len(old) == 0 case.
I was thinking to copy codes from go source, but a lot of code to be copied on.

trotterdylan

Thanks for working on this! I have a few suggestions.

trotterdylan · 2017-01-20T17:37:19Z

+	s := toStrUnsafe(args[0]).Value()
+	old := toStrUnsafe(args[1]).Value()
+	sub := toStrUnsafe(args[2]).Value()
+	// CPython only, pypy supposed to be same as Go.


I'm a little confused about the meaning of this comment. Could you try to make the message a little clearer?

trotterdylan · 2017-01-20T17:38:11Z

+	old := toStrUnsafe(args[1]).Value()
+	sub := toStrUnsafe(args[2]).Value()
+	// CPython only, pypy supposed to be same as Go.
+	if len(s) == 0 && len(old) == 0 && n >= 0 {


I'm probably overlooking something but don't we always want to return "" when len(s) is zero?

"".replace("", "x") returns "x", but "".replace("", "x",1) return "" in CPython, so it is needed imo.

trotterdylan · 2017-01-20T17:38:44Z

+			buf.WriteString(sub)
+			n--
+		}
+		for i := 0; i < len(s); i++ {


Store len(s) as a temporary numBytes or something so it's not recomputed every iteration.

trotterdylan · 2017-01-20T17:40:20Z

+	if n == 0 {
+		return NewStr(s).ToObject(), nil
+	}
+	if len(old) == 0 {


Maybe switch the conditional to len(old) > 0 and do the strings.Replace() call to exit the function early. That would mean this big block of code doesn't have to be indented and it's probably easier to read.

trotterdylan · 2017-01-20T17:40:39Z

+	if n == 0 {
+		return NewStr(s).ToObject(), nil
+	}
+	if len(old) == 0 {


Worth adding a comment about why we do all this extra work instead of using strings.Replace()

trotterdylan · 2017-01-20T17:42:51Z

+	if n == 0 {
+		return NewStr(s).ToObject(), nil
+	}
+	if len(old) == 0 {


Prefer old == "" to len(old) == 0

trotterdylan · 2017-01-20T17:47:50Z

+				n--
+			} else {
+				i++
+				if i < len(s) {


Can we do this branch after the loop? I think the code would read more clearly: do the required replacements, then write the remainder of the string.

I think things would be tidier if your loop condition dictated when you exit the loop:

i := 0 for n > 0 && i < numBytes { // .. replacement logic i++ n-- }

As written it's not immediately obvious what the termination condition is.

Great idea, thank you.

S-YOU · 2017-01-21T01:39:31Z

Updated those, Please take a look again.

trotterdylan

This is great! Thanks for the implementation and for the thorough test cases. Merging.

trotterdylan suggested changes Jan 19, 2017

View reviewed changes

Implement str.replace

f8b2d01

S-YOU force-pushed the add_str_replace branch from aa6852d to 4c614b0 Compare January 19, 2017 17:52

test utf-8 replace; fix some issues

b68b367

S-YOU force-pushed the add_str_replace branch from 4c614b0 to b68b367 Compare January 19, 2017 17:53

S-YOU changed the title ~~[WIP] Implement str.replace~~ Implement str.replace Jan 19, 2017

add one more test case with long max replace

828540d

support blank string replace on utf8 string

b5976ca

trotterdylan suggested changes Jan 20, 2017

View reviewed changes

updates based on reviews.

7ded7c4

trotterdylan approved these changes Jan 21, 2017

View reviewed changes

trotterdylan merged commit a09bb56 into google:master Jan 21, 2017

Conversation

S-YOU commented Jan 19, 2017

Uh oh!

trotterdylan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

S-YOU Jan 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

S-YOU commented Jan 19, 2017

Uh oh!

trotterdylan commented Jan 19, 2017

Uh oh!

S-YOU commented Jan 19, 2017

Uh oh!

trotterdylan commented Jan 19, 2017 via email

Uh oh!

S-YOU commented Jan 19, 2017

Uh oh!

trotterdylan commented Jan 19, 2017 via email

Uh oh!

S-YOU commented Jan 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

trotterdylan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

S-YOU Jan 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

S-YOU commented Jan 21, 2017

Uh oh!

trotterdylan left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

S-YOU Jan 19, 2017 •

edited

Loading

S-YOU commented Jan 20, 2017 •

edited

Loading

S-YOU Jan 21, 2017 •

edited

Loading