code review 940041: Added 'ReadRune' to strings.Reader

Added 'ReadRune' to strings.Reader

[Kyle C, 2010-04-20 17:35:32]

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

I'd like you to review this change.

[rsc, 2010-04-21 03:13:25]

Looks reasonable, but it needs a method comment.
I'd suggest lifting the one from bytes.Buffer's ReadRune:

$ godoc bytes ReadRune
// ReadRune reads and returns the next UTF-8-encoded
// Unicode code point from the buffer.
// If no bytes are available, the error returned is os.EOF.
// If the bytes are an erroneous UTF-8 encoding, it
// consumes one byte and returns U+FFFD, 1.
func (b *Buffer) ReadRune() (r int, size int, err os.Error)
$

Also, returning -1 is atypical for Go functions.
Typically if there's nothing to return, they return the zero value.
The fact that it's an error is signaled instead by err != nil.

For example:
$ godoc -src bytes ReadRune
...
func (b *Buffer) ReadRune() (r int, size int, err os.Error) {
    if b.off >= len(b.buf) {

        b.Truncate(0)
        return 0, 0, os.EOF
    }
...

Russ

[Kyle C, 2010-04-21 04:01:03]

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

Please take another look.

[Kyle C, 2010-04-21 04:04:44]

Also updated to have the same behavior on invalid utf8 that
bytes.Buffer.ReadRune() has,
as I assume we'd want ReadRune to work the same in both places.
Then again, looks like bufio.Reader.ReadRune() doesn't work that way.
Is this difference in behavior intentional?

On Tue, Apr 20, 2010 at 9:01 PM, <consalus@gmail.com> wrote:

> Hello golang-dev@googlecode.com (cc: golang-dev@googlegroups.com),
>
> Please take another look.
>
>
>
> http://codereview.appspot.com/940041/show
>

[rsc, 2010-04-21 04:13:18]

On Tue, Apr 20, 2010 at 21:04, Kyle Consalus <consalus@gmail.com> wrote:
> Also updated to have the same behavior on invalid utf8 that
> bytes.Buffer.ReadRune() has,
> as I assume we'd want ReadRune to work the same in both places.
> Then again, looks like bufio.Reader.ReadRune() doesn't work that way.
> Is this difference in behavior intentional?

At first glance they look the same to me.
What's different?

Russ

[rsc1, 2010-04-21 04:18:32]

Looks pretty good.

http://codereview.appspot.com/940041/diff/4003/7001
File src/pkg/strings/reader.go (right):

http://codereview.appspot.com/940041/diff/4003/7001#newcode7
src/pkg/strings/reader.go:7: import "os"
Use factored import

import (
    "os"
    "utf8"
)

http://codereview.appspot.com/940041/diff/4003/7001#newcode10
src/pkg/strings/reader.go:10: // A Reader satisfies calls to Read and ReadByte
by
... to Read, ReadByte, and ReadRune by

http://codereview.appspot.com/940041/diff/4003/7002
File src/pkg/strings/strings_test.go (right):

http://codereview.appspot.com/940041/diff/4003/7002#newcode12
src/pkg/strings/strings_test.go:12: "os"
sort into place

http://codereview.appspot.com/940041/diff/4003/7002#newcode582
src/pkg/strings/strings_test.go:582: test_strings := []string{"", abcd, faces,
commas}
testStrings

http://codereview.appspot.com/940041/diff/4003/7002#newcode590
src/pkg/strings/strings_test.go:590: } else if e != nil {
drop else after if body that does not finish:

}
if e != nil {

http://codereview.appspot.com/940041/diff/4003/7002#newcode591
src/pkg/strings/strings_test.go:591: t.Errorf("Expect no error, got %s",
e.String())
t.Errorf("reading %q: %s", s, e)

http://codereview.appspot.com/940041/diff/4003/7002#newcode597
src/pkg/strings/strings_test.go:597: t.Errorf("strings.Reader(%q).ReadRune() =
%q, want %q", s, res, s)
t.Errorf("Reader(%q).ReadRune produced %q", s, res)

the "strings." is redundant in this package,
and ReadRune didn't directly = %q.

[Kyle C, 2010-04-21 04:49:17]

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

Please take another look.

[rsc1, 2010-04-21 04:57:58]

LGTM

Thanks.

[rsc, 2010-04-21 05:18:28]

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

strings: add ReadRune to Reader

R=rsc
CC=golang-dev
http://codereview.appspot.com/940041

Committer: Russ Cox <rsc@golang.org>