Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(420)

Issue 13004046: code review 13004046: encoding/xml: flush buffer after encoding token (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
10 years, 7 months ago by Dominik Honnef
Modified:
10 years, 6 months ago
Reviewers:
adg
CC:
rsc, bradfitz, adg, golang-dev
Visibility:
Public.

Description

encoding/xml: flush buffer after encoding token

Patch Set 1 #

Patch Set 2 : diff -r 6d2a2628859d https://code.google.com/p/go #

Patch Set 3 : diff -r 6d2a2628859d https://code.google.com/p/go #

Patch Set 4 : diff -r 6d2a2628859d https://code.google.com/p/go #

Patch Set 5 : diff -r fd947a6bbee7 https://code.google.com/p/go #

Patch Set 6 : diff -r fd947a6bbee7 https://code.google.com/p/go #

Patch Set 7 : diff -r fd947a6bbee7 https://code.google.com/p/go #

Total comments: 4

Patch Set 8 : diff -r fd947a6bbee7 https://code.google.com/p/go #

Total comments: 2

Patch Set 9 : diff -r fd947a6bbee7 https://code.google.com/p/go #

Unified diffs Side-by-side diffs Delta from patch set Stats (+10 lines, -2 lines) Patch
M src/pkg/encoding/xml/marshal.go View 1 2 2 chunks +1 line, -2 lines 0 comments Download
M src/pkg/encoding/xml/marshal_test.go View 1 2 3 4 5 6 7 8 1 chunk +9 lines, -0 lines 0 comments Download

Messages

Total messages: 17
Dominik Honnef
Hello rsc@golang.org (cc: golang-dev@googlegroups.com), I'd like you to review this change to https://code.google.com/p/go
10 years, 7 months ago (2013-08-17 05:12:07 UTC) #1
bradfitz
All the tests already pass. On Aug 16, 2013 10:12 PM, <dominik.honnef@gmail.com> wrote: > Reviewers: ...
10 years, 7 months ago (2013-08-17 08:32:13 UTC) #2
Dominik Honnef
On 2013/08/17 08:32:13, bradfitz wrote: > All the tests already pass. There are no tests ...
10 years, 7 months ago (2013-08-17 17:16:30 UTC) #3
adg
Here's your test (not tested ;-): func TestEncodeTokenFlush(t *testing.T) { var b bytes.Buffer enc := ...
10 years, 7 months ago (2013-08-18 22:25:01 UTC) #4
Dominik Honnef
Hello rsc@golang.org, bradfitz@golang.org, adg@golang.org (cc: golang-dev@googlegroups.com), Please take another look.
10 years, 7 months ago (2013-08-18 22:41:59 UTC) #5
adg
https://codereview.appspot.com/13004046/diff/18001/src/pkg/encoding/xml/marshal_test.go File src/pkg/encoding/xml/marshal_test.go (right): https://codereview.appspot.com/13004046/diff/18001/src/pkg/encoding/xml/marshal_test.go#newcode1081 src/pkg/encoding/xml/marshal_test.go:1081: delete blank line https://codereview.appspot.com/13004046/diff/18001/src/pkg/encoding/xml/marshal_test.go#newcode1084 src/pkg/encoding/xml/marshal_test.go:1084: if g, w := ...
10 years, 7 months ago (2013-08-18 23:30:07 UTC) #6
Dominik Honnef
PTAL https://codereview.appspot.com/13004046/diff/18001/src/pkg/encoding/xml/marshal_test.go File src/pkg/encoding/xml/marshal_test.go (right): https://codereview.appspot.com/13004046/diff/18001/src/pkg/encoding/xml/marshal_test.go#newcode1081 src/pkg/encoding/xml/marshal_test.go:1081: On 2013/08/18 23:30:08, adg wrote: > delete blank ...
10 years, 7 months ago (2013-08-18 23:39:11 UTC) #7
adg
Sorry, one last nit https://codereview.appspot.com/13004046/diff/14002/src/pkg/encoding/xml/marshal_test.go File src/pkg/encoding/xml/marshal_test.go (right): https://codereview.appspot.com/13004046/diff/14002/src/pkg/encoding/xml/marshal_test.go#newcode1084 src/pkg/encoding/xml/marshal_test.go:1084: t.Errorf("Encoder wrote %#v, want %#v", ...
10 years, 7 months ago (2013-08-18 23:55:55 UTC) #8
Dominik Honnef
Told you I wasn't the right person ;-) https://codereview.appspot.com/13004046/diff/14002/src/pkg/encoding/xml/marshal_test.go File src/pkg/encoding/xml/marshal_test.go (right): https://codereview.appspot.com/13004046/diff/14002/src/pkg/encoding/xml/marshal_test.go#newcode1084 src/pkg/encoding/xml/marshal_test.go:1084: t.Errorf("Encoder ...
10 years, 7 months ago (2013-08-18 23:58:48 UTC) #9
adg
LGTM
10 years, 7 months ago (2013-08-19 00:05:09 UTC) #10
adg
On 19 August 2013 09:58, <dominik.honnef@gmail.com> wrote: > Told you I wasn't the right person ...
10 years, 7 months ago (2013-08-19 00:10:16 UTC) #11
adg
*** Submitted as https://code.google.com/p/go/source/detail?r=5db14b33d6ef *** encoding/xml: flush buffer after encoding token R=rsc, bradfitz, adg CC=golang-dev ...
10 years, 7 months ago (2013-08-19 00:16:01 UTC) #12
rsc
This is wrong and should be undone. Flushing after every token negates nearly the entire ...
10 years, 6 months ago (2013-09-03 20:30:07 UTC) #13
Dominik Honnef
On 2013/09/03 20:30:07, rsc wrote: > This is wrong and should be undone. > > ...
10 years, 6 months ago (2013-09-03 20:38:38 UTC) #14
Dominik Honnef
On 2013/09/03 20:38:38, Dominik Honnef wrote: > On 2013/09/03 20:30:07, rsc wrote: > > This ...
10 years, 6 months ago (2013-09-03 20:52:14 UTC) #15
adg
On 4 September 2013 06:30, Russ Cox <rsc@golang.org> wrote: > This is wrong and should ...
10 years, 6 months ago (2013-09-04 00:44:49 UTC) #16
rsc
10 years, 6 months ago (2013-09-05 16:03:28 UTC) #17
On Tue, Sep 3, 2013 at 8:44 PM, Andrew Gerrand <adg@golang.org> wrote:

>
> On 4 September 2013 06:30, Russ Cox <rsc@golang.org> wrote:
>
>> This is wrong and should be undone.
>>
>> Flushing after every token negates nearly the entire advantage of
>> buffering writes in the first place.
>>
>
> Without some change it is impossible to use EncodeToken correctly.
>

That's not strictly true. If you use it during the implementation of a
custom MarshalXML method, it works correctly without the flush, because
there will be a flush when the whole XML stream is done.

EncodeToken is not used by other parts of package xml. The buffering in the
> encoder is obviously useful for the implementation of the Encode method,
> but users of EncodeToken should perhaps be doing their own buffering anyway.
>
> If you don't like this change, then we should add a Flush method to
> Encoder, as Dominik suggests.
>

I agree that there should be a Flush.

Russ
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b