code review 4066043: encoding/line: make it an io.Reader too

encoding/line: make it an io.Reader too

[bradfitz, 2011-01-25 17:01:34]

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

I'd like you to review this change.

[bradfitzgoog, 2011-01-25 17:05:23]

Background:  I wanted a line reader to read some HTTP headers, but once
those are done I want to just io.Copy() the rest.

On Tue, Jan 25, 2011 at 9:01 AM, <bradfitz@golang.org> wrote:

> Reviewers: agl1,
>
> Message:
> Hello agl1 (cc: golang-dev@googlegroups.com),
>
> I'd like you to review this change.
>
>
> Description:
> encoding/line: make it an io.Reader too
>
> Please review this at http://codereview.appspot.com/4066043/
>
> Affected files:
>  M src/pkg/encoding/line/line.go
>  M src/pkg/encoding/line/line_test.go
>
>
> Index: src/pkg/encoding/line/line.go
> ===================================================================
> --- a/src/pkg/encoding/line/line.go
> +++ b/src/pkg/encoding/line/line.go
> @@ -28,6 +28,27 @@
>        }
>  }
>
> +// Read reads from any buffered data past the last line read, or from the
> underlying
> +// io.Reader if the buffer is empty.
> +func (l *Reader) Read(p []byte) (n int, err os.Error) {
> +       l.removeConsumedFromBuffer()
> +       if len(l.buf) > 0 {
> +               n = copy(p, l.buf)
> +               err = nil
> +               l.consumed += n
> +               return
> +       }
> +       return l.in.Read(p)
> +}
> +
> +func (l *Reader) removeConsumedFromBuffer() {
> +       if l.consumed > 0 {
> +               n := copy(l.buf, l.buf[l.consumed:])
> +               l.buf = l.buf[:n]
> +               l.consumed = 0
> +       }
> +}
> +
>  // ReadLine tries to return a single line, not including the end-of-line
> bytes.
>  // If the line was found to be longer than the maximum length then
> isPrefix is
>  // set and the beginning of the line is returned. The rest of the line
> will be
> @@ -36,11 +57,7 @@
>  // the Reader and is only valid until the next call to ReadLine. ReadLine
>  // either returns a non-nil line or it returns an error, never both.
>  func (l *Reader) ReadLine() (line []byte, isPrefix bool, err os.Error) {
> -       if l.consumed > 0 {
> -               n := copy(l.buf, l.buf[l.consumed:])
> -               l.buf = l.buf[:n]
> -               l.consumed = 0
> -       }
> +       l.removeConsumedFromBuffer()
>
>        if len(l.buf) == 0 && l.err != nil {
>                err = l.err
> Index: src/pkg/encoding/line/line_test.go
> ===================================================================
> --- a/src/pkg/encoding/line/line_test.go
> +++ b/src/pkg/encoding/line/line_test.go
> @@ -6,6 +6,7 @@
>
>  import (
>        "bytes"
> +       "io"
>        "os"
>        "testing"
>  )
> @@ -87,3 +88,23 @@
>                t.Errorf("bad result for third line: %x", line)
>        }
>  }
> +
> +func TestReadAfterLines(t *testing.T) {
> +       line1 := "line1"
> +       restData := "line2\nline 3\n"
> +       inbuf := bytes.NewBuffer([]byte(line1 + "\n" + restData))
> +       outbuf := new(bytes.Buffer)
> +       maxLineLength := len(line1) + len(restData)/2
> +       l := NewReader(inbuf, maxLineLength)
> +       line, isPrefix, err := l.ReadLine()
> +       if isPrefix || err != nil || string(line) != line1 {
> +               t.Errorf("bad result for first line: isPrefix=%v err=%v
> line=%q", isPrefix, err, string(line))
> +       }
> +       n, err := io.Copy(outbuf, l)
> +       if int(n) != len(restData) || err != nil {
> +               t.Errorf("bad result for Read: n=%d err=%v", n, err)
> +       }
> +       if outbuf.String() != restData {
> +               t.Errorf("bad result for Read: got %q; expected %q",
> outbuf.String(), restData)
> +       }
> +}
>
>
>

[agl1, 2011-01-25 17:08:48]

LGTM
On Jan 25, 2011 12:01 PM, <bradfitz@golang.org> wrote:
> Reviewers: agl1,
>
> Message:
> Hello agl1 (cc: golang-dev@googlegroups.com),
>
> I'd like you to review this change.
>
>
> Description:
> encoding/line: make it an io.Reader too
>
> Please review this at http://codereview.appspot.com/4066043/
>
> Affected files:
> M src/pkg/encoding/line/line.go
> M src/pkg/encoding/line/line_test.go
>
>
> Index: src/pkg/encoding/line/line.go
> ===================================================================
> --- a/src/pkg/encoding/line/line.go
> +++ b/src/pkg/encoding/line/line.go
> @@ -28,6 +28,27 @@
> }
> }
>
> +// Read reads from any buffered data past the last line read, or from the

> underlying
> +// io.Reader if the buffer is empty.
> +func (l *Reader) Read(p []byte) (n int, err os.Error) {
> + l.removeConsumedFromBuffer()
> + if len(l.buf) > 0 {
> + n = copy(p, l.buf)
> + err = nil
> + l.consumed += n
> + return
> + }
> + return l.in.Read(p)
> +}
> +
> +func (l *Reader) removeConsumedFromBuffer() {
> + if l.consumed > 0 {
> + n := copy(l.buf, l.buf[l.consumed:])
> + l.buf = l.buf[:n]
> + l.consumed = 0
> + }
> +}
> +
> // ReadLine tries to return a single line, not including the end-of-line
> bytes.
> // If the line was found to be longer than the maximum length then
> isPrefix is
> // set and the beginning of the line is returned. The rest of the line
> will be
> @@ -36,11 +57,7 @@
> // the Reader and is only valid until the next call to ReadLine. ReadLine
> // either returns a non-nil line or it returns an error, never both.
> func (l *Reader) ReadLine() (line []byte, isPrefix bool, err os.Error) {
> - if l.consumed > 0 {
> - n := copy(l.buf, l.buf[l.consumed:])
> - l.buf = l.buf[:n]
> - l.consumed = 0
> - }
> + l.removeConsumedFromBuffer()
>
> if len(l.buf) == 0 && l.err != nil {
> err = l.err
> Index: src/pkg/encoding/line/line_test.go
> ===================================================================
> --- a/src/pkg/encoding/line/line_test.go
> +++ b/src/pkg/encoding/line/line_test.go
> @@ -6,6 +6,7 @@
>
> import (
> "bytes"
> + "io"
> "os"
> "testing"
> )
> @@ -87,3 +88,23 @@
> t.Errorf("bad result for third line: %x", line)
> }
> }
> +
> +func TestReadAfterLines(t *testing.T) {
> + line1 := "line1"
> + restData := "line2\nline 3\n"
> + inbuf := bytes.NewBuffer([]byte(line1 + "\n" + restData))
> + outbuf := new(bytes.Buffer)
> + maxLineLength := len(line1) + len(restData)/2
> + l := NewReader(inbuf, maxLineLength)
> + line, isPrefix, err := l.ReadLine()
> + if isPrefix || err != nil || string(line) != line1 {
> + t.Errorf("bad result for first line: isPrefix=%v err=%v line=%q",
> isPrefix, err, string(line))
> + }
> + n, err := io.Copy(outbuf, l)
> + if int(n) != len(restData) || err != nil {
> + t.Errorf("bad result for Read: n=%d err=%v", n, err)
> + }
> + if outbuf.String() != restData {
> + t.Errorf("bad result for Read: got %q; expected %q", outbuf.String(),
> restData)
> + }
> +}
>
>

[rsc, 2011-01-25 17:56:25]

LGTM

[rsc, 2011-01-25 17:58:09]

On Tue, Jan 25, 2011 at 12:05, Brad Fitzpatrick <bradfitz@google.com> wrote:
> Background:  I wanted a line reader to read some HTTP headers, but once
> those are done I want to just io.Copy() the rest.

I think it's good to have Read here, but for your
specific problem it might also work to use
net/textproto's Reader, which already has
an HTTP header reader.

Russ

[agl1, 2011-01-26 15:00:09]

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

encoding/line: make it an io.Reader too

R=agl1, bradfitzwork, rsc
CC=golang-dev
http://codereview.appspot.com/4066043

Committer: Adam Langley <agl@golang.org>