code review 7181053: encoding/json: add Decoder.Buffered accessor to get ove...

encoding/json: add Decoder.Buffered accessor to get overread data

Otherwise it's impossible to know how much data from the
json.Decoder's underlying Reader was actually consumed.

The old fix from golang.org/issue/1955 just added docs. This
provides an actual mechanism.

Update issue 1955

[bradfitz, 2013-01-23 01:08:41]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://go.googlecode.com/hg/

[adg, 2013-01-23 01:25:41]

https://codereview.appspot.com/7181053/diff/3001/src/pkg/encoding/json/stream.go
File src/pkg/encoding/json/stream.go (right):

https://codereview.appspot.com/7181053/diff/3001/src/pkg/encoding/json/stream...
src/pkg/encoding/json/stream.go:62: // Remaining returns a reader of the
buffered data remaining
s/buffered  //

https://codereview.appspot.com/7181053/diff/3001/src/pkg/encoding/json/stream...
src/pkg/encoding/json/stream.go:63: // in the Decoder's buffer, along with the
source reader's
"followed by the rest of the source data." ?

[bradfitz, 2013-01-23 01:27:46]

https://codereview.appspot.com/7181053/diff/3001/src/pkg/encoding/json/stream.go
File src/pkg/encoding/json/stream.go (right):

https://codereview.appspot.com/7181053/diff/3001/src/pkg/encoding/json/stream...
src/pkg/encoding/json/stream.go:62: // Remaining returns a reader of the
buffered data remaining
On 2013/01/23 01:25:42, adg wrote:
> s/buffered  //

I dunno. That word's kinda the whole point. "slop", "overread", "accidentally
read". I think "buffered" says it best.

I'll let Russ pick the wording if he doesn't hate the design.

I'd be happy with any solution that lets me know accurately which bytes were
used and which weren't.

[adg, 2013-01-23 01:39:57]

https://codereview.appspot.com/7181053/diff/3001/src/pkg/encoding/json/stream.go
File src/pkg/encoding/json/stream.go (right):

https://codereview.appspot.com/7181053/diff/3001/src/pkg/encoding/json/stream...
src/pkg/encoding/json/stream.go:62: // Remaining returns a reader of the
buffered data remaining
On 2013/01/23 01:27:46, bradfitz wrote:
> On 2013/01/23 01:25:42, adg wrote:
> > s/buffered  //
> 
> I dunno. That word's kinda the whole point. "slop", "overread", "accidentally
> read". I think "buffered" says it best.
> 
> I'll let Russ pick the wording if he doesn't hate the design.
> 
> I'd be happy with any solution that lets me know accurately which bytes were
> used and which weren't.

You have "the buffered data remaining in the Decoder's buffer." I suggest "the
data remaining in the Decoder's buffer." If the data is in the buffer, it's
buffered no?

https://codereview.appspot.com/7181053/diff/3001/src/pkg/encoding/json/stream...
src/pkg/encoding/json/stream.go:66: return
io.MultiReader(bytes.NewReader(dec.buf), dec.r)
Another option is to let the caller do the MultiReader thing themselves. I'm in
two minds. This is probably cleaner for the majority case, but less flexible
than just returning the buffered bytes. It also hides the underlying type of the
original reader. Hm.

[bradfitz, 2013-01-23 01:49:49]

https://codereview.appspot.com/7181053/diff/3001/src/pkg/encoding/json/stream.go
File src/pkg/encoding/json/stream.go (right):

https://codereview.appspot.com/7181053/diff/3001/src/pkg/encoding/json/stream...
src/pkg/encoding/json/stream.go:62: // Remaining returns a reader of the
buffered data remaining
On 2013/01/23 01:39:57, adg wrote:
> On 2013/01/23 01:27:46, bradfitz wrote:
> > On 2013/01/23 01:25:42, adg wrote:
> > > s/buffered  //
> > 
> > I dunno. That word's kinda the whole point. "slop", "overread",
"accidentally
> > read". I think "buffered" says it best.
> > 
> > I'll let Russ pick the wording if he doesn't hate the design.
> > 
> > I'd be happy with any solution that lets me know accurately which bytes were
> > used and which weren't.
> 
> You have "the buffered data remaining in the Decoder's buffer." I suggest "the
> data remaining in the Decoder's buffer." If the data is in the buffer, it's
> buffered no?
> 

oh, yeah.  sorry, i wrote this all on a bumpy bus and my brain always goes to
mush.

https://codereview.appspot.com/7181053/diff/3001/src/pkg/encoding/json/stream...
src/pkg/encoding/json/stream.go:66: return
io.MultiReader(bytes.NewReader(dec.buf), dec.r)
On 2013/01/23 01:39:57, adg wrote:
> Another option is to let the caller do the MultiReader thing themselves. I'm
in
> two minds. This is probably cleaner for the majority case, but less flexible
> than just returning the buffered bytes. It also hides the underlying type of
the
> original reader. Hm.

I don't care about the user losing their reader's concrete type: they were the
ones who provided it to us anyway with NewDecoder.

And we could just return the buffered slop too.  I don't care.  I thought this
was cuter, in that this reader could take over for the other one.

[rsc, 2013-01-23 02:55:34]

I am a little confused about why what's there isn't good enough. That is,
why not pass in an io.ByteReader like in TestLimitedBuffering?

[adg, 2013-01-23 02:55:35]

On 23 January 2013 12:49, <bradfitz@golang.org> wrote:

> And we could just return the buffered slop too.  I don't care.  I
> thought this was cuter, in that this reader could take over for the
> other one.
>

It's definitely cuter. I've just never needed to do this so I'm not sure
what the user wants.

[bradfitz, 2013-01-23 03:19:06]

Likely I'm stupid and missed that. Will look later.
 On Jan 22, 2013 6:55 PM, "Russ Cox" <rsc@golang.org> wrote:

> I am a little confused about why what's there isn't good enough. That is,
> why not pass in an io.ByteReader like in TestLimitedBuffering?
>
>
>

[bradfitz, 2013-01-23 23:43:07]

Russ, there is no TestLimitedBuffering.

Maybe you're referring to https://codereview.appspot.com/5623053/ which was
never submitted due to performance concerns.

My CL doesn't change the performance of the read loop.

Do you want me to finish 5623053 and have two read loops, one for big reads
and one for byte reads?


On Tue, Jan 22, 2013 at 7:19 PM, Brad Fitzpatrick <bradfitz@golang.org>wrote:

> Likely I'm stupid and missed that. Will look later.
>  On Jan 22, 2013 6:55 PM, "Russ Cox" <rsc@golang.org> wrote:
>
>> I am a little confused about why what's there isn't good enough. That is,
>> why not pass in an io.ByteReader like in TestLimitedBuffering?
>>
>>
>>

[bradfitz, 2013-01-29 00:25:24]

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

Please take another look.

[rsc, 2013-01-29 00:26:25]

LGTM

[bradfitz, 2013-01-29 00:31:48]

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

encoding/json: add Decoder.Buffered accessor to get overread data

Otherwise it's impossible to know how much data from the
json.Decoder's underlying Reader was actually consumed.

The old fix from golang.org/issue/1955 just added docs. This
provides an actual mechanism.

Update issue 1955

R=golang-dev, adg, rsc
CC=golang-dev
https://codereview.appspot.com/7181053