|
|
|
encoding/xml: make sure Encoder.Encode reports Write errors.
Fixes issue 4112.
Patch Set 1 #Patch Set 2 : diff -r 760f1daa6700 https://code.google.com/p/go #
Total comments: 6
Patch Set 3 : diff -r 3e65e5b411d5 https://code.google.com/p/go #
Total messages: 15
|
osaingre
Hi, I'd like you to review that Go 1.1 issue fix. Thanks, Olivier
|
12 years, 12 months ago (2013年01月13日 21:22:19 UTC) #1 | ||||||||||||||||||||||||||||||||||||||||
Hi, I'd like you to review that Go 1.1 issue fix. Thanks, Olivier
Why don't I see this issue on http://gocodereview.appspot.com ? https://codereview.appspot.com/7085053/diff/1001/src/pkg/encoding/xml/marshal.go File src/pkg/encoding/xml/marshal.go (right): https://codereview.appspot.com/7085053/diff/1001/src/pkg/encoding/xml/marshal... src/pkg/encoding/xml/marshal.go:249: if err := EscapeText(p, []byte(val.String())); err != nil { the printer is backed by a bufio.Writer and hence has sticky errors: any error invalidates all future writes and should be returned by the p.cachedWriteError() below. Why doesn't it work?
On Wed, Jan 16, 2013 at 10:34 PM, <remyoudompheng@gmail.com> wrote: > Why don't I see this issue on http://gocodereview.appspot.com ? > I think gocodereview is mail based and golang-dev isn't on the CC list.
Maybe this review was created by hand without the aide of hg mail On Thu, Jan 17, 2013 at 8:01 PM, Daniel Morsing <daniel.morsing@gmail.com> wrote: > On Wed, Jan 16, 2013 at 10:34 PM, <remyoudompheng@gmail.com> wrote: >> Why don't I see this issue on http://gocodereview.appspot.com ? >> > > I think gocodereview is mail based and golang-dev isn't on the CC list.
On 2013年01月17日 09:03:21, dfc wrote: > Maybe this review was created by hand without the aide of hg mail > > On Thu, Jan 17, 2013 at 8:01 PM, Daniel Morsing > <mailto:daniel.morsing@gmail.com> wrote: > > On Wed, Jan 16, 2013 at 10:34 PM, <mailto:remyoudompheng@gmail.com> wrote: > >> Why don't I see this issue on http://gocodereview.appspot.com ? > >> > > > > I think gocodereview is mail based and golang-dev isn't on the CC list. In fact, I left the Reviewer and CC fields blanks (as specified in Contributing Guidelines) when doing 'hg change'. But no notifications were sent to golang-dev. So I edited the code review to add golang-dev and puslished+mailed.
https://codereview.appspot.com/7085053/diff/1001/src/pkg/encoding/xml/marshal.go File src/pkg/encoding/xml/marshal.go (right): https://codereview.appspot.com/7085053/diff/1001/src/pkg/encoding/xml/marshal... src/pkg/encoding/xml/marshal.go:249: if err := EscapeText(p, []byte(val.String())); err != nil { On 2013年01月16日 21:34:13, remyoudompheng wrote: > the printer is backed by a bufio.Writer and hence has sticky errors: any error > invalidates all future writes and should be returned by the p.cachedWriteError() > below. > > Why doesn't it work? Agreed. https://codereview.appspot.com/7085053/diff/1001/src/pkg/encoding/xml/marshal... src/pkg/encoding/xml/marshal.go:260: if err := EscapeText(p, bytes); err != nil { While you are here, make this val.Slice(0, val.Len()).Bytes() and delete the three lines above. https://codereview.appspot.com/7085053/diff/1001/src/pkg/encoding/xml/marshal... src/pkg/encoding/xml/marshal.go:293: return err This might be worth keeping because it exits the loop early.
https://codereview.appspot.com/7085053/diff/1001/src/pkg/encoding/xml/marshal.go File src/pkg/encoding/xml/marshal.go (right): https://codereview.appspot.com/7085053/diff/1001/src/pkg/encoding/xml/marshal... src/pkg/encoding/xml/marshal.go:260: if err := EscapeText(p, bytes); err != nil { On 2013年01月18日 22:03:24, rsc wrote: > While you are here, make this val.Slice(0, val.Len()).Bytes() and delete the > three lines above. If I do that, it'll panic on arrays because of the reflect.ValueOf upstream, in Encode().
https://codereview.appspot.com/7085053/diff/1001/src/pkg/encoding/xml/marshal.go File src/pkg/encoding/xml/marshal.go (right): https://codereview.appspot.com/7085053/diff/1001/src/pkg/encoding/xml/marshal... src/pkg/encoding/xml/marshal.go:260: if err := EscapeText(p, bytes); err != nil { Not sure I understand. At the least, you can do var bytes []bytes if val.CanAddr() { bytes = val.Slice(0, val.Len()) } else { bytes = make([]byte, val.Len()) reflect.Copy(reflect.ValueOf(bytes), val) }
Hello remyoudompheng@gmail.com, daniel.morsing@gmail.com, dave@cheney.net, rsc@golang.org (cc: golang-dev@googlegroups.com), I'd like you to review this change to https://code.google.com/p/go
LGTM
Please complete a CLA as described at golang.org/doc/contribute.html#copyright. Thanks. Russ
On 2013年02月19日 15:48:50, rsc wrote: > Please complete a CLA as described at > golang.org/doc/contribute.html#copyright. > > Thanks. > Russ Cool, thanks. I agreed and submitted yesterday. Though I got no confirmation.
Yeah, there is no visible confirmation. Once the paperwork is done on the Google side a committer will propose a change which adds your details to CONTRIBUTORS and AUTHORS. On Wed, Feb 20, 2013 at 7:33 PM, <osaingre@gmail.com> wrote: > On 2013年02月19日 15:48:50, rsc wrote: >> >> Please complete a CLA as described at >> golang.org/doc/contribute.html#copyright. > > >> Thanks. >> Russ > > Cool, thanks. > I agreed and submitted yesterday. Though I got no confirmation. > > > https://codereview.appspot.com/7085053/
*** Submitted as https://code.google.com/p/go/source/detail?r=ccecfdaf3719 *** encoding/xml: make sure Encoder.Encode reports Write errors. Fixes issue 4112. R=remyoudompheng, daniel.morsing, dave, rsc CC=golang-dev https://codereview.appspot.com/7085053 Committer: Brad Fitzpatrick <bradfitz@golang.org>