code review 634042: Added 'CopyToken' func to safely create a copy of an xm...

Added 'CopyToken' func to safely create a copy of an xml.Token

[Kyle C, 2010-03-18 20:48:36]

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

I'd like you to review this change.

[rsc1, 2010-03-18 20:51:47]

Looks reasonable.
Maybe it should be a method on Token?

Russ

[Kyle C, 2010-03-18 21:03:31]

By my reading of the language spec, methods can't be declared on interface
types like Token.
If this is not the case, then a method on Token would probably be the way to
go.


On Thu, Mar 18, 2010 at 1:51 PM, Russ Cox <rsc@google.com> wrote:

> Looks reasonable.
> Maybe it should be a method on Token?
>
> Russ
>

[rsc1, 2010-03-18 21:14:21]

Sorry, I'd forgotten that Token was an interface.

Are you copying a generic Token or a specific one?
If the latter, you could use Copy directly if you
convert it to a concrete type first.

Alternately, we could change the Copy methods
to return Token instead of the concrete types,
but that's inconvenient in a different way.

Russ

[Kyle C, 2010-03-18 21:22:09]

On Thu, Mar 18, 2010 at 2:14 PM, Russ Cox <rsc@google.com> wrote:

> Sorry, I'd forgotten that Token was an interface.
>
> Are you copying a generic Token or a specific one?
> If the latter, you could use Copy directly if you
> convert it to a concrete type first.
>

I'm copying a generic Token. This is for the general case where someone
wants to keep copies of parsed tokens
for later processing.
(I created this because I wrote some code that emits tokens to a channel,
and it resulted in some corruption. I had
CopyToken in my code, but since it depends on what types might be Tokens, I
figured it should live in the xml package.)


>
> Alternately, we could change the Copy methods
> to return Token instead of the concrete types,
> but that's inconvenient in a different way.
>
Yeah. I also considered adding "CopyToken() Token" to Token interface, then
for each Token type just having CopyToken()
call Copy() (or return the receiver, if appropriate). That has some
benefits, but I concluded that just having CopyToken as a free function is
probably simpler.


>
> Russ
>

[rsc1, 2010-03-19 23:37:00]

LGTM

please fix the comments and run hg mail again.

also, please complete the CLA as described at
http://golang.org/doc/contribute.html

thanks

http://codereview.appspot.com/634042/diff/2001/3001
File src/pkg/xml/xml.go (right):

http://codereview.appspot.com/634042/diff/2001/3001#newcode110
src/pkg/xml/xml.go:110: // Returns a deep copy of a Token.
deep is debatable.

// CopyToken returns a copy of a Token.

the rest is not necessary - it duplicates the text below.

http://codereview.appspot.com/634042/diff/2001/3001#newcode209
src/pkg/xml/xml.go:209: // call to Token.  To acquire a copy of the bytes, call
the token's
// ..., call CopyToken
// or the token's Copy method.

[Kyle C, 2010-03-19 23:48:07]

http://codereview.appspot.com/634042/diff/2001/3001
File src/pkg/xml/xml.go (right):

http://codereview.appspot.com/634042/diff/2001/3001#newcode110
src/pkg/xml/xml.go:110: // Returns a deep copy of a Token.
On 2010/03/19 23:37:01, rsc1 wrote:
> deep is debatable.
> 
> // CopyToken returns a copy of a Token.
> 
> the rest is not necessary - it duplicates the text below.
> 

Done.

http://codereview.appspot.com/634042/diff/2001/3001#newcode209
src/pkg/xml/xml.go:209: // call to Token.  To acquire a copy of the bytes, call
the token's
On 2010/03/19 23:37:01, rsc1 wrote:
> // ..., call CopyToken
> // or the token's Copy method.
> 

Done.

[Kyle C, 2010-03-19 23:48:59]

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

Please take another look.

[rsc1, 2010-03-19 23:53:03]

LGTM

thanks

[rsc, 2010-03-23 16:43:26]

*** Submitted as http://code.google.com/p/go/source/detail?r=dc4b7964940d ***

xml: add CopyToken

R=rsc
CC=golang-dev
http://codereview.appspot.com/634042

Committer: Russ Cox <rsc@golang.org>