Code review - Issue 6868044: code review 6868044: encoding/xml: namespace handlinghttps://codereview.appspot.com/2014-04-09T01:57:35+00:00rietveld
Message from unknown
2012-12-02T01:06:08+00:00cjyarurn:md5:e5792515a2af74c64f08f2eba6e5357b
Message from rsc@golang.org
2012-12-09T17:21:59+00:00rscurn:md5:fbf082f187a74765f4c439d6291adca1
Just found this linked in issue 3526. Please run hg mail to send to golang-dev. Thanks.
Message from chris@cjones.org
2012-12-10T01:29:58+00:00chris_cjones.orgurn:md5:6a0f416dad17acc629f984fae48fffdf
maxwell$ hg stat
M src/pkg/encoding/xml/atom_test.go
M src/pkg/encoding/xml/marshal.go
M src/pkg/encoding/xml/marshal_test.go
M src/pkg/encoding/xml/read.go
M src/pkg/encoding/xml/read_test.go
M src/pkg/encoding/xml/typeinfo.go
M src/pkg/encoding/xml/xml.go
M src/pkg/encoding/xml/xml_test.go
A src/pkg/encoding/xml/namespace.go
? patch.3526.2
maxwell$ hg mail
abort: no files changed
On 12/9/2012 10:21 AM, rsc@golang.org wrote:
> Just found this linked in issue 3526. Please run hg mail to send to
> golang-dev. Thanks.
>
>
> https://codereview.appspot.com/6868044/
Message from unknown
2012-12-10T02:05:54+00:00cjyarurn:md5:608a3b18990a9ff57487bc53b9fd1dbf
Message from chris.jones.yar@gmail.com
2012-12-10T02:06:03+00:00cjyarurn:md5:5abcd64734016fa44402655e3cc9e04b
Hello golang-dev@googlegroups.com, rsc@golang.org, chris@cjones.org (cc: golang-dev@googlegroups.com),
I'd like you to review this change to
https://code.google.com/p/go
Message from rsc@golang.org
2012-12-11T18:31:27+00:00rscurn:md5:b739780d6201a7be147dc191d56a3b26
I'd like to see what Gustavo thinks if he's around.
There's a lot of new API here.
Message from unknown
2012-12-14T06:36:03+00:00cjyarurn:md5:b98a2d0b4de4098d1af4a7d583054b06
Message from chris.jones.yar@gmail.com
2012-12-14T06:36:09+00:00cjyarurn:md5:e6a2bbadcd6161698a573eb14ca624dc
Hello rsc@golang.org, chris@cjones.org, n13m3y3r@gmail.com (cc: golang-dev@googlegroups.com),
Please take another look.
Message from unknown
2012-12-14T06:41:08+00:00cjyarurn:md5:063fb96b4b514efebbab1ed7e3bd18f0
Message from chris.jones.yar@gmail.com
2012-12-14T06:41:14+00:00cjyarurn:md5:21c6413bce69e7752bd670fd5fd102fb
Hello rsc@golang.org, chris@cjones.org, n13m3y3r@gmail.com (cc: golang-dev@googlegroups.com),
Please take another look.
Message from unknown
2012-12-16T22:26:20+00:00cjyarurn:md5:317719493315a3bfac832e33e6d753b0
Message from chris.jones.yar@gmail.com
2012-12-16T22:26:26+00:00cjyarurn:md5:6f966d545cf0950b7984a616aad20dd2
Hello rsc@golang.org, chris@cjones.org, n13m3y3r@gmail.com (cc: golang-dev@googlegroups.com),
Please take another look.
Message from unknown
2012-12-17T02:54:04+00:00cjyarurn:md5:bf17866f055533785d3505dc0d9981b5
Message from chris.jones.yar@gmail.com
2012-12-17T02:54:11+00:00cjyarurn:md5:1ce23fd8d7b32ec3b9e8ffa882ed270b
Hello rsc@golang.org, chris@cjones.org, n13m3y3r@gmail.com (cc: golang-dev@googlegroups.com),
Please take another look.
Message from rsc@golang.org
2012-12-17T16:19:47+00:00rscurn:md5:3c83ce415a9724f0859bfb7e71cf4656
I would like to drop Context from the API. Can we do that?
It seems to me that even if you're in the middle of a document you can
just introduce new prefixes as needed.
Russ
Message from rsc@golang.org
2012-12-17T18:54:36+00:00rscurn:md5:447f6dfdc0fb170429f22a386a8a6f90
Perhaps, but perhaps not.
I think that if you need to send a <foo> in the jabber:client name
space, you can always send
<foo xmlns="jabber:client">
instead of needing to know what the outer version is called.
Russ
Message from rsc@golang.org
2012-12-22T14:57:10+00:00rscurn:md5:ad3ad09c4e129adc136129e7bd5946cd
It's easy to handle the parsing half. Use the xml Decoder to read the
first element, and it will have the appropriate context.
For the printing half, I'd still like to avoid adding new API until we
have a demonstrated need. Any XMPP implementation must parse XML, and
what I've suggested is valid XML. It's true that it adds the 22 bytes
or so to each message, but I've seen what XMPP messages look like: no
one will notice 22 extra bytes. Also, if you're doing XMPP over TLS
it's all getting compressed anyway.
Russ
Message from unknown
2012-12-28T22:10:16+00:00cjyarurn:md5:8da4fa1134e2c9444ca1c6224ce72e53
Message from unknown
2012-12-28T22:12:36+00:00cjyarurn:md5:460e81ee9f3cc52bfcf0713cdff44252
Message from chris.jones.yar@gmail.com
2012-12-28T22:12:45+00:00cjyarurn:md5:1f74aae9e5959fa8fe055f6e7e18d661
Hello rsc@golang.org, chris@cjones.org, n13m3y3r@gmail.com (cc: golang-dev@googlegroups.com),
I'd like you to review this change to
https://code.google.com/p/go
Message from rsc@golang.org
2013-01-18T22:24:56+00:00rscurn:md5:280b2405d1255843b5a15b195b8861d7
Can we please split this into two CLs, one for encoding and one for decoding? Perhaps move the decoding to the new CL.
I don't believe the decoder should need any changes at all, except perhaps to set the default xml and xmlns meanings.
https://codereview.appspot.com/6868044/diff/23001/src/pkg/encoding/xml/xml.go
File src/pkg/encoding/xml/xml.go (right):
https://codereview.appspot.com/6868044/diff/23001/src/pkg/encoding/xml/xml.go#newcode165
src/pkg/encoding/xml/xml.go:165: // context provides the namespace context, for use in the
I'm a little confused. The current decoder already tracks name spaces. Does the current code not work? I believe it does, in which case we can leave this file alone.
Message from chris.jones.yar@gmail.com
2013-01-21T04:46:08+00:00cjyarurn:md5:43d07388aeb122eeaa9b22ce82a7d99e
It would be awkward to split this into two CLs, but I'll do it if you really want me to. Both Marshal and Unmarshal need to be fixed, and some of the fixes are common to both (typeinfo.go).
Having said that, I need to clean this up a bit more and submit yet another revision of it. The CL has evolved since I originally wrote it, and I haven't been keeping it all neat.
https://codereview.appspot.com/6868044/diff/23001/src/pkg/encoding/xml/xml.go
File src/pkg/encoding/xml/xml.go (right):
https://codereview.appspot.com/6868044/diff/23001/src/pkg/encoding/xml/xml.go#newcode165
src/pkg/encoding/xml/xml.go:165: // context provides the namespace context, for use in the
Issue 3526 isn't very clear on this: The lack of support for attribute namespaces is common to both unmarshal and marshal.
Message from rsc@golang.org
2013-01-22T22:15:15+00:00rscurn:md5:0165cd7bca8a50963285102db437cd9d
I believe that Unmarshal is handling name spaces just fine and does not
need changes. If you disagree, please point out specific things that it is
doing wrong. This CL appears to be replacing correct code with new (perhaps
correct, but perhaps not) code. Instead of wholesale code replacement I
would prefer to fix the code that is there, unless it is truly
unsalvageable. I haven't seen any indication of that yet. I suggest to move
the Unmarshal changes - if any are needed! - to a separate CL.
Marshal, on the other hand, has never attempted to do something useful with
name spaces. I can believe that it needs help.
Russ
Message from unknown
2013-01-31T05:33:39+00:00cjyarurn:md5:639b659f5f49d13de431dc6d325678a1
Message from chris.jones.yar@gmail.com
2013-01-31T05:33:44+00:00cjyarurn:md5:44d774f07e2ccec08f2bc02bd4dd3e2d
Hello rsc@golang.org, n13m3y3r@gmail.com (cc: golang-dev@googlegroups.com),
Please take another look.
Message from mendelsongusmao@gmail.com
2013-11-27T15:41:02+00:00mendelurn:md5:55b89c8bca3f9391724a732d16dbafc6
On 2013/01/31 05:33:44, cjyar wrote:
> Hello mailto:rsc@golang.org, mailto:n13m3y3r@gmail.com (cc: mailto:golang-dev@googlegroups.com),
>
> Please take another look.
Hi! I have a bug to report.
The scenario is: two distinct structs (therefore two distinct XML documents) sharing a struct.
The first call to Unmarshal will be OK with all contents being retrieved from the document.
The second call to Unmarshal won't retrieve the contents from the shared structure.
If tinfoMap is cleaned right after the call to Unmarshal, everything works but this doesn't seem to be OK as there's a performance penalty.
I'm not familiar with Google Code and Rietveld Code Review Tool, so I created a gist with a test to demonstrate the issue:
https://gist.github.com/MendelGusmao/7677446
Thanks.
Message from dave@cheney.net
2013-11-30T01:16:11+00:00dfcurn:md5:7ae2ea1b2bc666eb2150fc2540dc6f29
@mendel this is not the appropriate forum for a bug report.
Please log issues here, http://golang.org/issue
On 2013/11/27 15:41:02, mendel wrote:
> On 2013/01/31 05:33:44, cjyar wrote:
> > Hello mailto:rsc@golang.org, mailto:n13m3y3r@gmail.com (cc:
> mailto:golang-dev@googlegroups.com),
> >
> > Please take another look.
>
> Hi! I have a bug to report.
>
> The scenario is: two distinct structs (therefore two distinct XML documents)
> sharing a struct.
> The first call to Unmarshal will be OK with all contents being retrieved from
> the document.
> The second call to Unmarshal won't retrieve the contents from the shared
> structure.
>
> If tinfoMap is cleaned right after the call to Unmarshal, everything works but
> this doesn't seem to be OK as there's a performance penalty.
>
> I'm not familiar with Google Code and Rietveld Code Review Tool, so I created a
> gist with a test to demonstrate the issue:
>
> https://gist.github.com/MendelGusmao/7677446
>
> Thanks.
Message from gobot@golang.org
2013-12-20T16:21:42+00:00goboturn:md5:a31571ace69867d24d7a09ac198753cd
Replacing golang-dev with golang-codereviews.
Message from rsc@golang.org
2014-04-09T01:57:35+00:00rscurn:md5:8e1f792b5b652aed3b2ec10fe3ad6ca5
R=close
The referenced issue is marked fixed.