code review 78120043: text/scanner: handle non-io.EOF errors

text/scanner: handle non-io.EOF errors

Currently Scan ignores an error returned from source if the number
of bytes source has read is 0.

Fixes issue 7594.

[ruiu, 2014-03-20 06:42:52]

Hello golang-codereviews@googlegroups.com,

I'd like you to review this change to
https://code.google.com/p/go

[bradfitz, 2014-03-20 16:41:12]

R=gri



On Wed, Mar 19, 2014 at 11:42 PM, <ruiu@google.com> wrote:

> Reviewers: golang-codereviews,
>
> Message:
> Hello golang-codereviews@googlegroups.com,
>
> I'd like you to review this change to
> https://code.google.com/p/go
>
>
> Description:
> text/scanner: handle non-io.EOF errors
>
> Currently Scan ignores an error returned from source if the number
> of bytes source has read is 0.
>
> Fixes issue 7594.
>
> Please review this at https://codereview.appspot.com/78120043/
>
> Affected files (+34, -3 lines):
>   M src/pkg/text/scanner/scanner.go
>   M src/pkg/text/scanner/scanner_test.go
>
>
> Index: src/pkg/text/scanner/scanner.go
> ===================================================================
> --- a/src/pkg/text/scanner/scanner.go
> +++ b/src/pkg/text/scanner/scanner.go
> @@ -240,6 +240,9 @@
>                         s.srcEnd = i + n
>                         s.srcBuf[s.srcEnd] = utf8.RuneSelf // sentinel
>                         if err != nil {
> +                               if err != io.EOF {
> +                                       s.error(err.Error())
> +                               }
>                                 if s.srcEnd == 0 {
>                                         if s.lastCharLen > 0 {
>                                                 // previous character was
> not EOF
> @@ -248,9 +251,6 @@
>                                         s.lastCharLen = 0
>                                         return EOF
>                                 }
> -                               if err != io.EOF {
> -                                       s.error(err.Error())
> -                               }
>                                 // If err == EOF, we won't be getting more
>                                 // bytes; break to avoid infinite loop. If
>                                 // err is something else, we don't know if
> Index: src/pkg/text/scanner/scanner_test.go
> ===================================================================
> --- a/src/pkg/text/scanner/scanner_test.go
> +++ b/src/pkg/text/scanner/scanner_test.go
> @@ -6,6 +6,7 @@
>
>  import (
>         "bytes"
> +       "errors"
>         "fmt"
>         "io"
>         "strings"
> @@ -462,6 +463,36 @@
>         testError(t, `/*/`, "1:4", "comment not terminated", EOF)
>  }
>
> +// An ErrorReader returns (0, <error>) where <error> is not an io.EOF.
> +type ErrorReader struct {
> +       msg string
> +}
> +
> +func (r *ErrorReader) Read(b []byte) (int, error) {
> +       return 0, errors.New(r.msg)
> +}
> +
> +func TestIOError(t *testing.T) {
> +       const msg = "ErrorReader error"
> +       s := new(Scanner).Init(&ErrorReader{msg})
> +       errorCalled := false
> +       s.Error = func(s *Scanner, m string) {
> +               if !errorCalled {
> +                       if m != msg {
> +                               t.Errorf("msg = %q, want %q", m, msg)
> +                       }
> +                       errorCalled = true
> +               }
> +       }
> +       tk := s.Scan()
> +       if tk != EOF {
> +               t.Errorf("tok = %s, want EOF", TokenString(tk))
> +       }
> +       if !errorCalled {
> +               t.Errorf("error handler not called")
> +       }
> +}
> +
>  func checkPos(t *testing.T, got, want Position) {
>         if got.Offset != want.Offset || got.Line != want.Line ||
> got.Column != want.Column {
>                 t.Errorf("got offset, line, column = %d, %d, %d; want %d,
> %d, %d",
>
>
> --
> You received this message because you are subscribed to the Google Groups
> "golang-codereviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to golang-codereviews+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.
>

[gri, 2014-03-21 22:51:25]

The fix looks good to me. Thanks.

Some nitpicks in the test.

https://codereview.appspot.com/78120043/diff/70001/src/pkg/text/scanner/scann...
File src/pkg/text/scanner/scanner_test.go (right):

https://codereview.appspot.com/78120043/diff/70001/src/pkg/text/scanner/scann...
src/pkg/text/scanner/scanner_test.go:466: // An ErrorReader returns (0, <error>)
where <error> is not an io.EOF.
simplify, no need for export

s/ErrorReader/errReader/

// An errReader returns (0, err) where err is not io.EOF.

https://codereview.appspot.com/78120043/diff/70001/src/pkg/text/scanner/scann...
src/pkg/text/scanner/scanner_test.go:468: msg string
remove msg - just use an empty struct

https://codereview.appspot.com/78120043/diff/70001/src/pkg/text/scanner/scann...
src/pkg/text/scanner/scanner_test.go:471: func (r *ErrorReader) Read(b []byte)
(int, error) {
func (errReader) Read ...

no need for receiver name, no need for pointer

https://codereview.appspot.com/78120043/diff/70001/src/pkg/text/scanner/scann...
src/pkg/text/scanner/scanner_test.go:472: return 0, errors.New(r.msg)
return 0, io.ErrNoProgress // some error that is not io.EOF

also, as a consequence, no need to import errors

https://codereview.appspot.com/78120043/diff/70001/src/pkg/text/scanner/scann...
src/pkg/text/scanner/scanner_test.go:476: const msg = "ErrorReader error"
no need for msg

https://codereview.appspot.com/78120043/diff/70001/src/pkg/text/scanner/scann...
src/pkg/text/scanner/scanner_test.go:477: s :=
new(Scanner).Init(&ErrorReader{msg})
Init(errReader{})

https://codereview.appspot.com/78120043/diff/70001/src/pkg/text/scanner/scann...
src/pkg/text/scanner/scanner_test.go:479: s.Error = func(s *Scanner, m string) {
s/m/msg/ for consistency with the parameter name in the api

https://codereview.appspot.com/78120043/diff/70001/src/pkg/text/scanner/scann...
src/pkg/text/scanner/scanner_test.go:481: if m != msg {
if want := io.ErrNoProgress.Error(); msg != want {
   t.Errorf("msg = %q, want %q", msg, want)
}

https://codereview.appspot.com/78120043/diff/70001/src/pkg/text/scanner/scann...
src/pkg/text/scanner/scanner_test.go:487: tk := s.Scan()
s/tk/tok/

for consistency with the rest of this file

[ruiu, 2014-03-21 23:59:52]

Thank you for reviewing. PTAL.

https://codereview.appspot.com/78120043/diff/70001/src/pkg/text/scanner/scann...
File src/pkg/text/scanner/scanner_test.go (right):

https://codereview.appspot.com/78120043/diff/70001/src/pkg/text/scanner/scann...
src/pkg/text/scanner/scanner_test.go:466: // An ErrorReader returns (0, <error>)
where <error> is not an io.EOF.
On 2014/03/21 22:51:26, gri wrote:
> simplify, no need for export
> 
> s/ErrorReader/errReader/
> 
> // An errReader returns (0, err) where err is not io.EOF.

Done.

https://codereview.appspot.com/78120043/diff/70001/src/pkg/text/scanner/scann...
src/pkg/text/scanner/scanner_test.go:468: msg string
On 2014/03/21 22:51:26, gri wrote:
> remove msg - just use an empty struct

Done.

https://codereview.appspot.com/78120043/diff/70001/src/pkg/text/scanner/scann...
src/pkg/text/scanner/scanner_test.go:471: func (r *ErrorReader) Read(b []byte)
(int, error) {
On 2014/03/21 22:51:26, gri wrote:
> func (errReader) Read ...
> 
> no need for receiver name, no need for pointer

Done.

https://codereview.appspot.com/78120043/diff/70001/src/pkg/text/scanner/scann...
src/pkg/text/scanner/scanner_test.go:472: return 0, errors.New(r.msg)
On 2014/03/21 22:51:26, gri wrote:
> return 0, io.ErrNoProgress // some error that is not io.EOF
> 
> also, as a consequence, no need to import errors

Done.

https://codereview.appspot.com/78120043/diff/70001/src/pkg/text/scanner/scann...
src/pkg/text/scanner/scanner_test.go:476: const msg = "ErrorReader error"
On 2014/03/21 22:51:26, gri wrote:
> no need for msg

Done.

https://codereview.appspot.com/78120043/diff/70001/src/pkg/text/scanner/scann...
src/pkg/text/scanner/scanner_test.go:477: s :=
new(Scanner).Init(&ErrorReader{msg})
On 2014/03/21 22:51:26, gri wrote:
> Init(errReader{})

Done.

https://codereview.appspot.com/78120043/diff/70001/src/pkg/text/scanner/scann...
src/pkg/text/scanner/scanner_test.go:479: s.Error = func(s *Scanner, m string) {
On 2014/03/21 22:51:26, gri wrote:
> s/m/msg/ for consistency with the parameter name in the api

Done.

https://codereview.appspot.com/78120043/diff/70001/src/pkg/text/scanner/scann...
src/pkg/text/scanner/scanner_test.go:481: if m != msg {
On 2014/03/21 22:51:26, gri wrote:
> if want := io.ErrNoProgress.Error(); msg != want {
>    t.Errorf("msg = %q, want %q", msg, want)
> }

Done.

https://codereview.appspot.com/78120043/diff/70001/src/pkg/text/scanner/scann...
src/pkg/text/scanner/scanner_test.go:487: tk := s.Scan()
On 2014/03/21 22:51:26, gri wrote:
> s/tk/tok/
> 
> for consistency with the rest of this file

Done.

[gri, 2014-03-22 00:05:34]

LGTM.

Thanks.

[gri, 2014-03-22 00:06:14]

*** Submitted as https://code.google.com/p/go/source/detail?r=5cacef3086fb ***

text/scanner: handle non-io.EOF errors

Currently Scan ignores an error returned from source if the number
of bytes source has read is 0.

Fixes issue 7594.

LGTM=gri
R=golang-codereviews, bradfitz, gri
CC=golang-codereviews
https://codereview.appspot.com/78120043

Committer: Robert Griesemer <gri@golang.org>