Issue 8845043: code review 8845043: io: explain what (0,nil) means from Read (Closed)
|
Created:
11 years, 11 months ago by r Modified:
11 years, 11 months ago Reviewers:
CC:
golang-dev, dsymonds, bradfitz Visibility:
Public. |
Descriptionio: explain what (0,nil) means from Read
Also add a new variable ErrNoProgress that io.Readers can use to
report ineffectual Read calls.
Fixes issue 5310.
Patch Set 1 #Patch Set 2 : diff -r fa6a760fcc63 https://code.google.com/p/go/ #
Total comments: 3
Patch Set 3 : diff -r f5f3d54cfadf https://code.google.com/p/go/ #
Total comments: 1
Patch Set 4 : diff -r f5f3d54cfadf https://code.google.com/p/go/ #MessagesTotal messages: 12
Hello golang-dev@googlegroups.com, I'd like you to review this change to https://code.google.com/p/go
Sign in to reply to this message.
"should usually" seems awfully wishy washy for something so specific. Perhaps // The caller should handle this situation and may treat it as end-of-file.
Sign in to reply to this message.
I really don't like this. I don't think (0, nil) should be documented as equivalent to EOF. I even think bufio.Scanner's behavior is wrong. In fact, I should (0, nil) should be explicitly documented as a no-op, and as such discouraged as it's useless. On Wed, Apr 17, 2013 at 5:42 PM, <r@golang.org> wrote: > Reviewers: golang-dev1, > > Message: > Hello golang-dev@googlegroups.com, > > I'd like you to review this change to > https://code.google.com/p/go > > > Description: > io: explain what (0,nil) means from Read > Fixes issue 5310. > > Please review this at https://codereview.appspot.**com/8845043/<https://codereview.appspot.com/8845... > > Affected files: > M src/pkg/io/io.go > > > Index: src/pkg/io/io.go > ==============================**==============================**======= > --- a/src/pkg/io/io.go > +++ b/src/pkg/io/io.go > @@ -51,6 +51,10 @@ > // return either err == EOF or err == nil. The next Read should > // return 0, EOF regardless. > // > +// In some cases, such as reading from an interactive terminal, > +// an implementation may return n==0 and err==nil. > +// The caller should usually treat this situation as end-of-file. > +// > // Callers should always process the n > 0 bytes returned before > // considering the error err. Doing so correctly handles I/O errors > // that happen after reading some bytes and also both of the > > > -- > > ---You received this message because you are subscribed to the Google > Groups "golang-dev" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to golang-dev+unsubscribe@**googlegroups.com<golang-dev%2Bunsubscribe@googlegrou... > . > For more options, visit https://groups.google.com/**groups/opt_out<https://groups.google.com/groups/o... > . > > >
Sign in to reply to this message.
I think bufio.Scanner is correct and io.Copy is wrong.
Sign in to reply to this message.
Wow. We break the rules. in os.File.Read:
if n == 0 && len(b) > 0 && e == nil {
return 0, io.EOF
}
So that means a zero-length read from a system call will be turned
into an EOF. This is not what Unix does; a Go program doesn't behave
like a C program.
Update coming.
Sign in to reply to this message.
Elaborate. I disagree. If it's a stream and there are no message boundaries, zero just advances the stream of bytes by zero. On Apr 17, 2013 9:06 PM, "Rob Pike" <r@golang.org> wrote: > I think bufio.Scanner is correct and io.Copy is wrong. >
Sign in to reply to this message.
Hello golang-dev@googlegroups.com, dsymonds@golang.org, bradfitz@golang.org (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
LGTM https://codereview.appspot.com/8845043/diff/7001/src/pkg/io/io.go File src/pkg/io/io.go (right): https://codereview.appspot.com/8845043/diff/7001/src/pkg/io/io.go#newcode59 src/pkg/io/io.go:59: // Implementations of Read are discouraged from returning a ditch the passive voice? s/are discouraged from returning/should not return/
Sign in to reply to this message.
LGTM as-is https://codereview.appspot.com/8845043/diff/7001/src/pkg/io/io.go File src/pkg/io/io.go (right): https://codereview.appspot.com/8845043/diff/7001/src/pkg/io/io.go#newcode59 src/pkg/io/io.go:59: // Implementations of Read are discouraged from returning a On 2013/04/18 04:52:00, dsymonds wrote: > ditch the passive voice? > > s/are discouraged from returning/should not return/ While I agree with the sentiment to remove passive voice, the problem with "should not" is that to most people (especially when written in lowercase) sounds like "must not". Saying SHOULD NOT in the RFC 2119 sense works, but requires all-caps, and we don't use that terminology style elsewhere. So I'm fine with this word as-is. https://codereview.appspot.com/8845043/diff/7001/src/pkg/io/io.go#newcode60 src/pkg/io/io.go:60: // zero byte count with a nil error, and callers should treat though this "should" could be a "must" while still being RFC 2119 and English correct. But "should" is also fine.
Sign in to reply to this message.
Hello golang-dev@googlegroups.com, dsymonds@golang.org, bradfitz@golang.org (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
https://codereview.appspot.com/8845043/diff/13001/src/pkg/io/io.go File src/pkg/io/io.go (right): https://codereview.appspot.com/8845043/diff/13001/src/pkg/io/io.go#newcode37 src/pkg/io/io.go:37: // ErrNoProgress is returned by some implementations implementations of? more like "users of". Perhaps: // ErrNoProgress is returned by some callers // of an io.Reader when many calls to Read have failed // to return any data or an error, usually the sign // of a broken io.Reader implementation. (also clarifying what is broken)
Sign in to reply to this message.
*** Submitted as https://code.google.com/p/go/source/detail?r=864889d2dcee *** io: explain what (0,nil) means from Read Also add a new variable ErrNoProgress that io.Readers can use to report ineffectual Read calls. Fixes issue 5310. R=golang-dev, dsymonds, bradfitz CC=golang-dev https://codereview.appspot.com/8845043
Sign in to reply to this message.
|
