code review 7085053: encoding/xml: make sure Encoder.Encode reports Write errors.

encoding/xml: make sure Encoder.Encode reports Write errors.

Fixes issue 4112.

[osaingre, 2013-01-13 21:22:19]

Hi,
I'd like you to review that Go 1.1 issue fix.
Thanks,
Olivier

[remyoudompheng, 2013-01-16 21:34:13]

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?

[DMorsing, 2013-01-17 09:01:55]

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.

[dave_cheney.net, 2013-01-17 09:03:21]

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.

[osaingre, 2013-01-17 21:41:47]

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.

[rsc, 2013-01-18 22:03:24]

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.

[osaingre, 2013-02-04 22:29:04]

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().

[rsc, 2013-02-08 20:12:12]

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)
}

[osaingre, 2013-02-18 22:32:02]

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

[rsc, 2013-02-19 15:44:36]

LGTM

[rsc, 2013-02-19 15:48:50]

Please complete a CLA as described at
golang.org/doc/contribute.html#copyright.

Thanks.
Russ

[osaingre, 2013-02-20 08:33:54]

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.

[dave_cheney.net, 2013-02-20 08:40:50]

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/

[bradfitz, 2013-02-20 22:41:25]

*** 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>

[remyoudompheng, 2013-07-20 21:33:17]

R=close