Issue 113060043: code review 113060043: bufio: make naming consistent and clearer (Closed)
Descriptionbufio: fix UnreadSlice followed by UnreadRune
Also, fix a write check in writeBuf and make some bounds checks simpler.
Patch Set 1 #Patch Set 2 : diff -r 67f9ef140028 https://code.google.com/p/go #Patch Set 3 : diff -r 67f9ef140028 https://code.google.com/p/go #Patch Set 4 : diff -r 67f9ef140028 https://code.google.com/p/go #Patch Set 5 : diff -r 67f9ef140028 https://code.google.com/p/go #Patch Set 6 : diff -r 67f9ef140028 https://code.google.com/p/go #
Total comments: 6
Patch Set 7 : diff -r 6866fbf95095 https://code.google.com/p/go #Patch Set 8 : diff -r 6866fbf95095 https://code.google.com/p/go #Patch Set 9 : diff -r 6866fbf95095 https://code.google.com/p/go #
Total comments: 4
Patch Set 10 : diff -r 5207b394de96 https://code.google.com/p/go #Patch Set 11 : diff -r 5207b394de96 https://code.google.com/p/go #Patch Set 12 : diff -r 5207b394de96 https://code.google.com/p/go #
Total comments: 1
Patch Set 13 : diff -r 5207b394de96 https://code.google.com/p/go #
Total comments: 3
Patch Set 14 : diff -r 99630144d7fc https://code.google.com/p/go #Patch Set 15 : diff -r 99630144d7fc https://code.google.com/p/go #
MessagesTotal messages: 21
Hello golang-codereviews@googlegroups.com, I'd like you to review this change to https://code.google.com/p/go
Sign in to reply to this message.
If you're going to do a mass renaming please do it in a separate CL to any code changes. You have thrown some small refactorings in here, which might be easy to check for correctness on their own, but are hard to see with all the other renaming-related changes going on.
Sign in to reply to this message.
On 2014/07/14 04:37:54, adg wrote: > If you're going to do a mass renaming please do it in a separate CL to any code > changes. You have thrown some small refactorings in here, which might be easy to > check for correctness on their own, but are hard to see with all the other > renaming-related changes going on. Sure, I'll make change this CL to only contain renaming. I currently uploaded those changes as different deltas, since I'm not completely familiar with hg codereview. Also regarding that, how do I upload a second CL so that the other CL is a prerequisite for it? Should I wait until the first one gets merged or should I upload the second CL as a series of deltas and later sync them with the main-line? Or is there some command I missed?
Sign in to reply to this message.
On 14 July 2014 16:41, <egonelbre@gmail.com> wrote: > Sure, I'll make change this CL to only contain renaming. I currently > uploaded those changes as different deltas, since I'm not completely > familiar with hg codereview. Also regarding that, how do I upload a > second CL so that the other CL is a prerequisite for it? > There's no technical way of doing this nicely, unfortunately. I usually just wait until the first CL is through, keeping the second one in an unmailed CL. :-/ > Should I wait > until the first one gets merged or should I upload the second CL as a > series of deltas and later sync them with the main-line? Or is there > some command I missed? >
Sign in to reply to this message.
On 2014/07/14 06:46:30, adg wrote: > On 14 July 2014 16:41, <mailto:egonelbre@gmail.com> wrote: > > > Sure, I'll make change this CL to only contain renaming. I currently > > uploaded those changes as different deltas, since I'm not completely > > familiar with hg codereview. Also regarding that, how do I upload a > > second CL so that the other CL is a prerequisite for it? > > > > There's no technical way of doing this nicely, unfortunately. I usually > just wait until the first CL is through, keeping the second one in an > unmailed CL. :-/ > > > > Should I wait > > until the first one gets merged or should I upload the second CL as a > > series of deltas and later sync them with the main-line? Or is there > > some command I missed? > > PTAL
Sign in to reply to this message.
Does not LGTM. These are mostly gratuitous changes for no benefit. Please leave alone. If you want to fix the one or two typos, fine. Renaming the incoming reading to src, perhaps. But leave the other names alone. Most of them are short _on_purpose_. Thanks. https://codereview.appspot.com/113060043/diff/90001/src/pkg/bufio/bufio.go File src/pkg/bufio/bufio.go (right): https://codereview.appspot.com/113060043/diff/90001/src/pkg/bufio/bufio.go#ne... src/pkg/bufio/bufio.go:34: start int // First non-processed byte in buf. Please no. there's a reason why these fields were called r and w: they are used everywhere. Yes, it may not be immediately clear (even though pretty obvious) that they stand for the r>ead and w>rite position, but anybody understanding this code will figure out right away, and then be annoyed by start and end all over the place. Please revert, this is a gratuitous change with no real benefit. Instead add a comment to the same effect, w/o the clutter everywhere: r, w int // buf read and write positions https://codereview.appspot.com/113060043/diff/90001/src/pkg/bufio/bufio.go#ne... src/pkg/bufio/bufio.go:50: rd, ok := src.(*Reader) leave alone https://codereview.appspot.com/113060043/diff/90001/src/pkg/bufio/bufio.go#ne... src/pkg/bufio/bufio.go:58: rd = &Reader{} leave alone https://codereview.appspot.com/113060043/diff/90001/src/pkg/bufio/bufio.go#ne... src/pkg/bufio/bufio.go:58: rd = &Reader{} rd is uncommon, leave alone https://codereview.appspot.com/113060043/diff/90001/src/pkg/bufio/bufio_test.go File src/pkg/bufio/bufio_test.go (right): https://codereview.appspot.com/113060043/diff/90001/src/pkg/bufio/bufio_test.... src/pkg/bufio/bufio_test.go:1015: func (rw errorReaderFromTest) Read(p []byte) (int, error) { why rw? where's the w coming from. Please leave alone, this is a gratuitous change. https://codereview.appspot.com/113060043/diff/90001/src/pkg/bufio/scan_test.go File src/pkg/bufio/scan_test.go (right): https://codereview.appspot.com/113060043/diff/90001/src/pkg/bufio/scan_test.g... src/pkg/bufio/scan_test.go:313: var errTest = errors.New("testError") leave this alone, testError is much nicer to read
Sign in to reply to this message.
On 2014/07/14 16:48:26, gri wrote:
> Does not LGTM.
>
> These are mostly gratuitous changes for no benefit.
I've encountered already two bugs which I could attribute to small names.
Accidentally left one fix in this patch, in writeBuf "if n < b.r-b.w {" should
be "if n < b.w-b.r {". The other is near the comment "Handle last byte, if
any.", lastRuneSize isn't being set.
I only made this pass due to bradfitz saying he would like to see a consistency
pass before a wholesale rewrite and adg saying not do the renaming+refactoring
at the same time. Of course, I could have misunderstood what consistency pass
exactly means.
> Please leave alone. If you
> want to fix the one or two typos, fine. Renaming the incoming reading to src,
> perhaps. But leave the other names alone. Most of them are short _on_purpose_.
Understood.
>
> Thanks.
>
> https://codereview.appspot.com/113060043/diff/90001/src/pkg/bufio/bufio.go
> File src/pkg/bufio/bufio.go (right):
>
>
https://codereview.appspot.com/113060043/diff/90001/src/pkg/bufio/bufio.go#ne...
> src/pkg/bufio/bufio.go:34: start int // First non-processed byte in buf.
> Please no. there's a reason why these fields were called r and w: they are
used
> everywhere. Yes, it may not be immediately clear (even though pretty obvious)
> that they stand for the r>ead and w>rite position, but anybody understanding
> this code will figure out right away, and then be annoyed by start and end all
> over the place. Please revert, this is a gratuitous change with no real
benefit.
> Instead add a comment to the same effect, w/o the clutter everywhere:
>
> r, w int // buf read and write positions
I was basing the names from Scanner, but now to review it's not exactly the same
use case as r/w.
>
>
https://codereview.appspot.com/113060043/diff/90001/src/pkg/bufio/bufio.go#ne...
> src/pkg/bufio/bufio.go:50: rd, ok := src.(*Reader)
> leave alone
>
>
https://codereview.appspot.com/113060043/diff/90001/src/pkg/bufio/bufio.go#ne...
> src/pkg/bufio/bufio.go:58: rd = &Reader{}
> leave alone
>
>
https://codereview.appspot.com/113060043/diff/90001/src/pkg/bufio/bufio.go#ne...
> src/pkg/bufio/bufio.go:58: rd = &Reader{}
> rd is uncommon, leave alone
I was using "rd" because there are places, where the "r" is used for rune. Also
rd was used as the field for storing io.Reader previously.
>
>
https://codereview.appspot.com/113060043/diff/90001/src/pkg/bufio/bufio_test.go
> File src/pkg/bufio/bufio_test.go (right):
>
>
https://codereview.appspot.com/113060043/diff/90001/src/pkg/bufio/bufio_test....
> src/pkg/bufio/bufio_test.go:1015: func (rw errorReaderFromTest) Read(p []byte)
> (int, error) {
> why rw? where's the w coming from. Please leave alone, this is a gratuitous
> change.
r/w receiver inconsistency was reported by golint, although using r instead of
rw makes sense.
>
> https://codereview.appspot.com/113060043/diff/90001/src/pkg/bufio/scan_test.go
> File src/pkg/bufio/scan_test.go (right):
>
>
https://codereview.appspot.com/113060043/diff/90001/src/pkg/bufio/scan_test.g...
> src/pkg/bufio/scan_test.go:313: var errTest = errors.New("testError")
> leave this alone, testError is much nicer to read
testError naming was reported by golint, it complains about errors that are not
being prefixed by "err".
Sign in to reply to this message.
On 2014/07/14 17:37:43, egon wrote:
> On 2014/07/14 16:48:26, gri wrote:
> > Does not LGTM.
> >
> > These are mostly gratuitous changes for no benefit.
>
> I've encountered already two bugs which I could attribute to small names.
> Accidentally left one fix in this patch, in writeBuf "if n < b.r-b.w {" should
> be "if n < b.w-b.r {". The other is near the comment "Handle last byte, if
> any.", lastRuneSize isn't being set.
>
> I only made this pass due to bradfitz saying he would like to see a
consistency
> pass before a wholesale rewrite and adg saying not do the renaming+refactoring
> at the same time. Of course, I could have misunderstood what consistency pass
> exactly means.
>
> > Please leave alone. If you
> > want to fix the one or two typos, fine. Renaming the incoming reading to
src,
> > perhaps. But leave the other names alone. Most of them are short
_on_purpose_.
>
> Understood.
I toned down the renaming (only src/dst) and added fix and test for ReadSlice,
UnreadRune case. Also fixed the writeBuf check.
>
> >
> > Thanks.
> >
> > https://codereview.appspot.com/113060043/diff/90001/src/pkg/bufio/bufio.go
> > File src/pkg/bufio/bufio.go (right):
> >
> >
>
https://codereview.appspot.com/113060043/diff/90001/src/pkg/bufio/bufio.go#ne...
> > src/pkg/bufio/bufio.go:34: start int // First non-processed byte in
buf.
> > Please no. there's a reason why these fields were called r and w: they are
> used
> > everywhere. Yes, it may not be immediately clear (even though pretty
obvious)
> > that they stand for the r>ead and w>rite position,
Just to clarify, I did not make the change to r and w because they weren't
clear, but rather because it's easy to miss mistakes similar to b.r-b.w vs
b.w-b.r.
Sign in to reply to this message.
Better, but I still think that some of the name changes don't help much. Leaving for r who wrote some of the original code for final judgement. https://codereview.appspot.com/113060043/diff/140001/src/pkg/bufio/bufio.go File src/pkg/bufio/bufio.go (right): https://codereview.appspot.com/113060043/diff/140001/src/pkg/bufio/bufio.go#n... src/pkg/bufio/bufio.go:33: src io.Reader // The reader provided by the client. // reader provided by client https://codereview.appspot.com/113060043/diff/140001/src/pkg/bufio/bufio.go#n... src/pkg/bufio/bufio.go:136: if b.w-b.r < n { if avail := b.w - b.r; avail < n { // not enough data available n = avail ... https://codereview.appspot.com/113060043/diff/140001/src/pkg/bufio/bufio.go#n... src/pkg/bufio/bufio.go:464: if n < 0 { I don't understand why this is correct. n should never be < 0. But it can be smaller than b.r-b.w.
Sign in to reply to this message.
On 2014/07/14 20:37:06, gri wrote: > Better, but I still think that some of the name changes don't help much. > > Leaving for r who wrote some of the original code for final judgement. > > https://codereview.appspot.com/113060043/diff/140001/src/pkg/bufio/bufio.go > File src/pkg/bufio/bufio.go (right): > > https://codereview.appspot.com/113060043/diff/140001/src/pkg/bufio/bufio.go#n... > src/pkg/bufio/bufio.go:33: src io.Reader // The reader provided by the > client. > // reader provided by client > > https://codereview.appspot.com/113060043/diff/140001/src/pkg/bufio/bufio.go#n... > src/pkg/bufio/bufio.go:136: if b.w-b.r < n { > if avail := b.w - b.r; avail < n { > // not enough data available > n = avail > ... > > In the initial version I also substituted b.w-b.r -> b.Buffered(), and added functions for b.w == b.r -> b.empty() and b.w-b.r >= len(b.buf) -> b.full(). I removed them to simplify the CL, does it make sense to readd them? https://codereview.appspot.com/113060043/diff/140001/src/pkg/bufio/bufio.go#n... > src/pkg/bufio/bufio.go:464: if n < 0 { > I don't understand why this is correct. n should never be < 0. But it can be > smaller than b.r-b.w. b.r-b.w makes even less sense, b.r-b.w is always <= 0. Panicing when n < b.w-b.r is also no appropriate, because io.Writer can write a few bytes at a time. The check for negative write is similar to checks for negative reads and seemes the most appropriate. Alternative would be to remove the check completely. Also I noticed one thing in b.Read that feels wrong: func (b *Reader) Read(p []byte) (n int, err error) { n = len(p) if n == 0 { return 0, b.readErr() } Shouldn't it return (0, nil) when there is content available. It seems that the b.err should only be returned after all the buffered content has been read.
Sign in to reply to this message.
Regarding the Read: Per the documentation of io: "Implementations of Read are discouraged from returning a zero byte count with a nil error, and callers should treat that situation as a no-op." That said, if the incoming buffer has 0 length there's not much one can do. It may return a prior error, but that also a bit odd. I suspect returning 0, nil might be ok. Or it could be 0 and an error that the buffer should not be the 0-length buffer. I'd leave it alone for now. https://codereview.appspot.com/113060043/diff/140001/src/pkg/bufio/bufio.go File src/pkg/bufio/bufio.go (right): https://codereview.appspot.com/113060043/diff/140001/src/pkg/bufio/bufio.go#n... src/pkg/bufio/bufio.go:464: if n < 0 { On 2014/07/14 20:37:06, gri wrote: > I don't understand why this is correct. n should never be < 0. But it can be > smaller than b.r-b.w. Ah, yes. My bad. Please ignore my comment.
Sign in to reply to this message.
On 2014/07/14 21:32:10, gri wrote: > Regarding the Read: > > Per the documentation of io: > > "Implementations of Read are discouraged from returning a zero byte count with a > nil error, and callers should treat that situation as a no-op." > > That said, if the incoming buffer has 0 length there's not much one can do. It > may return a prior error, but that also a bit odd. I think that the bufio.Reader should work as similarly to the incoming reader as possible; if the incoming reader has that behavior then also should bufio.Reader. As an example if the underlying reader returns n > 0 and io.EOF from Read, then the results from Read will look very weird: http://play.golang.org/p/85np5ge5HX > > I suspect returning 0, nil might be ok. Or it could be 0 and an error that the > buffer should not be the 0-length buffer. I'd leave it alone for now. > > https://codereview.appspot.com/113060043/diff/140001/src/pkg/bufio/bufio.go > File src/pkg/bufio/bufio.go (right): > > https://codereview.appspot.com/113060043/diff/140001/src/pkg/bufio/bufio.go#n... > src/pkg/bufio/bufio.go:464: if n < 0 { > On 2014/07/14 20:37:06, gri wrote: > > I don't understand why this is correct. n should never be < 0. But it can be > > smaller than b.r-b.w. > > Ah, yes. My bad. Please ignore my comment.
Sign in to reply to this message.
On Mon, Jul 14, 2014 at 5:32 PM, <gri@golang.org> wrote: > Regarding the Read: > > Per the documentation of io: > > "Implementations of Read are discouraged from returning a zero byte > count with a nil error, and callers should treat that situation as a > no-op." > > That said, if the incoming buffer has 0 length there's not much one can > do. It may return a prior error, but that also a bit odd. > Note, there is an un-triaged issue for this: https://code.google.com/p/go/issues/detail?id=8317 > > I suspect returning 0, nil might be ok. Or it could be 0 and an error > that the buffer should not be the 0-length buffer. I'd leave it alone > for now.
Sign in to reply to this message.
https://codereview.appspot.com/113060043/diff/200001/src/pkg/bufio/bufio.go File src/pkg/bufio/bufio.go (right): https://codereview.appspot.com/113060043/diff/200001/src/pkg/bufio/bufio.go#n... src/pkg/bufio/bufio.go:139: err = b.readErr() I noticed that Peek won't preserve the original error from the underlying reader. So sequence rdr.Read(x), rdr.Peek(n), rdr.Read(y), may give different result than just rdr.Read(x), rdr.Read(y). This is somewhat similar to the other readErr() case.
Sign in to reply to this message.
In retrospect I like you to undo all name changes: There are other bufio CL's in flight, and there's an issue pending for a careful rewrite or at least cleanup of this package. The name changes are internal and don't provide any immediate benefit but require extra work in merging CL. Also, arguably rd might be a better field name than src since this is about buffering an io.Reader rd rather than consuming some source src. Similar for dst. Please make this a CL about the actual concrete bug fixes (and adjust the CL desc accordingly). That way we separate the concerns of name changes and bug fixes. If we want to consider name changes later, we can make that a separate CL and decide independently. Thanks.
Sign in to reply to this message.
On 2014/07/16 22:06:41, gri wrote: > In retrospect I like you to undo all name changes: There are other bufio CL's in > flight, and there's an issue pending for a careful rewrite or at least cleanup > of this package. I was starting with that task, but then noticed some bugs in the process. For refactoring I usually start with name changes because it's easier to refactor and to spot mistakes. > The name changes are internal and don't provide any immediate > benefit but require extra work in merging CL. Understood. > > Also, arguably rd might be a better field name than src since this is about > buffering an io.Reader rd rather than consuming some source src. Similar for > dst. I chose src instead of rd because there is already bufio.Reader, which means it became too easy to confuse b bufio.Reader and rd io.Reader. So, calling one the src, although not fully correct, makes it possible to pass "the telephone test". > > Please make this a CL about the actual concrete bug fixes (and adjust the CL > desc accordingly). That way we separate the concerns of name changes and bug > fixes. If we want to consider name changes later, we can make that a separate CL > and decide independently. Makes sense. What shall I do about the b.err not being preserved in Peek and Read? Different CL & new issue or something else? > > Thanks.
Sign in to reply to this message.
LGTM A couple of minor nitpicks. Please address those and we're good to go. Thanks for bearing with me. - gri https://codereview.appspot.com/113060043/diff/220001/src/pkg/bufio/bufio.go File src/pkg/bufio/bufio.go (right): https://codereview.appspot.com/113060043/diff/220001/src/pkg/bufio/bufio.go#n... src/pkg/bufio/bufio.go:33: rd io.Reader // reader provided by the client. remove period at end - not a real sentence https://codereview.appspot.com/113060043/diff/220001/src/pkg/bufio/bufio.go#n... src/pkg/bufio/bufio.go:463: panic(errors.New("bufio: writer returned negative count from Write")) panic(errNegativeCount) ? https://codereview.appspot.com/113060043/diff/220001/src/pkg/bufio/bufio_test.go File src/pkg/bufio/bufio_test.go (right): https://codereview.appspot.com/113060043/diff/220001/src/pkg/bufio/bufio_test... src/pkg/bufio/bufio_test.go:471: _, err = r.ReadSlice(0x0) s/0x0/0/ 0 is good enough
Sign in to reply to this message.
On 2014/07/17 21:28:30, gri wrote: > LGTM > > A couple of minor nitpicks. Please address those and we're good to go. > > Thanks for bearing with me. Nothing to worry about, all comments/discussions were fair; and I'm still getting used to the process and hg codereview anyway :). > - gri > > https://codereview.appspot.com/113060043/diff/220001/src/pkg/bufio/bufio.go > File src/pkg/bufio/bufio.go (right): > > https://codereview.appspot.com/113060043/diff/220001/src/pkg/bufio/bufio.go#n... > src/pkg/bufio/bufio.go:33: rd io.Reader // reader provided by the > client. > remove period at end - not a real sentence > > https://codereview.appspot.com/113060043/diff/220001/src/pkg/bufio/bufio.go#n... > src/pkg/bufio/bufio.go:463: panic(errors.New("bufio: writer returned negative > count from Write")) > panic(errNegativeCount) > > ? > > https://codereview.appspot.com/113060043/diff/220001/src/pkg/bufio/bufio_test.go > File src/pkg/bufio/bufio_test.go (right): > > https://codereview.appspot.com/113060043/diff/220001/src/pkg/bufio/bufio_test... > src/pkg/bufio/bufio_test.go:471: _, err = r.ReadSlice(0x0) > s/0x0/0/ > > 0 is good enough
Sign in to reply to this message.
*** Submitted as https://code.google.com/p/go/source/detail?r=96441aa31723 *** bufio: fix UnreadSlice followed by UnreadRune Also, fix a write check in writeBuf and make some bounds checks simpler. LGTM=gri R=golang-codereviews, adg, gri, r, minux CC=golang-codereviews https://codereview.appspot.com/113060043 Committer: Robert Griesemer <gri@golang.org>
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
