code review 953041: json: Marshal, Unmarshal using new scanner

json: Marshal, Unmarshal using new scanner

[rsc, 2010-04-21 00:32:28]

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

I'd like you to review this change.

[r, 2010-04-21 00:53:37]

http://codereview.appspot.com/953041/diff/2001/3004
File src/pkg/json/decode.go (right):

http://codereview.appspot.com/953041/diff/2001/3004#newcode103
src/pkg/json/decode.go:103: if _, ok := r.(runtime.Error); ok {
encode's error handler should make the same check

http://codereview.appspot.com/953041/diff/2001/3004#newcode145
src/pkg/json/decode.go:145: //		panic(1)
??

http://codereview.appspot.com/953041/diff/2001/3004#newcode671
src/pkg/json/decode.go:671: 
why these blank lines? it's not consistent style

http://codereview.appspot.com/953041/diff/2001/3004#newcode791
src/pkg/json/decode.go:791: // unquote converts a quoted JSON string literal s
into an actual string t.
comment why you can't use strconv.Unquote

http://codereview.appspot.com/953041/diff/2001/3004#newcode841
src/pkg/json/decode.go:841: // TODO(rsc): UTF-16 decoding of surrogate pairs
[sic]
do it now.

http://codereview.appspot.com/953041/diff/2001/3005
File src/pkg/json/decode_test.go (right):

http://codereview.appspot.com/953041/diff/2001/3005#newcode106
src/pkg/json/decode_test.go:106: if c == ' ' || c == '\t' || c == '\n' {
another test asked about \r too. be consistent.
how about
if isSpace return -1
return c

http://codereview.appspot.com/953041/diff/2001/3005#newcode118
src/pkg/json/decode_test.go:118: Int64   int64
there should be a tag in here somewhere

http://codereview.appspot.com/953041/diff/2001/3006
File src/pkg/json/encode.go (right):

http://codereview.appspot.com/953041/diff/2001/3006#newcode29
src/pkg/json/encode.go:29: // replaced by the encoding of the Unicode
replacement character.
s/.$/ U+FFFD./

http://codereview.appspot.com/953041/diff/2001/3006#newcode54
src/pkg/json/encode.go:54: // an infinite recursion.
TODO: catch this one day?

[rsc, 2010-04-21 23:15:33]

Done except

> http://codereview.appspot.com/953041/diff/2001/3005#newcode118
> src/pkg/json/decode_test.go:118: Int64   int64
> there should be a tag in here somewhere

Do you mean a struct field tag like "foo" ?
The json code doesn't consult struct field tags at all.

> http://codereview.appspot.com/953041/diff/2001/3006#newcode54
> src/pkg/json/encode.go:54: // an infinite recursion.
> TODO: catch this one day?

Maybe, but maybe not.  It's very expensive.

Russ

[r2, 2010-04-21 23:21:15]

On Apr 21, 2010, at 4:15 PM, Russ Cox wrote:

> Done except
>
>> http://codereview.appspot.com/953041/diff/2001/3005#newcode118
>> src/pkg/json/decode_test.go:118: Int64   int64
>> there should be a tag in here somewhere
>
> Do you mean a struct field tag like "foo" ?
> The json code doesn't consult struct field tags at all.

then fix the doc comment please.

-rob

[rsc, 2010-04-21 23:25:52]

PTAL

Added the code for tag handling instead.

[r, 2010-04-21 23:29:14]

LGTM

[rsc, 2010-04-21 23:40:57]

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

json: Marshal, Unmarshal using new scanner

R=r
CC=golang-dev
http://codereview.appspot.com/953041

[cw, 2010-04-22 08:38:34]

nag

http://codereview.appspot.com/953041/diff/12002/22006
File src/pkg/json/encode.go (right):

http://codereview.appspot.com/953041/diff/12002/22006#newcode57
src/pkg/json/encode.go:57: func Marshal(v interface{}) ([]byte, os.Error) {
before this worked with an io.Writer, now it returns a byte slice

this means for very large objects more memory is needed (transiently)

where i used to have:

  err = json.MarshalIndent(b, details, " ")

(b provides io.Writer)

I now have to get a byte slice back and use WriteFile

[rsc, 2010-04-22 08:41:31]

yes, that's what it means.
it also means that if the code encounters an
error halfway through marshalling, you don't
end up with a half-written file.

you win some, you lose some.

russ

[cw, 2010-04-22 09:02:53]

On Thu, Apr 22, 2010 at 01:41:26AM -0700, Russ Cox wrote:

> yes, that's what it means.
> it also means that if the code encounters an
> error halfway through marshalling, you don't
> end up with a half-written file.

i write to a temporary file and rename when it's done, that way i
don't get hosed configuration

> you win some, you lose some.

true

[cw, 2010-04-27 18:41:22]

i'm wondering if we can't have a common interface for json and gobs (and
potentially other things later)

http://codereview.appspot.com/953041/diff/12002/22004
File src/pkg/json/decode.go (left):

http://codereview.appspot.com/953041/diff/12002/22004#oldcode44
src/pkg/json/decode.go:44: }
Forgive me if I'm missing something here.

But is there not an opportunity to have a common Unmarshaler interface for gobs
and json (and potentially other things in the future)?

[rsc, 2010-04-27 18:51:32]

> But is there not an opportunity to have a common Unmarshaler interface
> for gobs and json (and potentially other things in the future)?

Sure but at a higher level.  All the top-level gob and json stuff
uses the same signature now.  (Compare godoc gob .*coder and godoc json .*coder)
This is about encoding a single item and is looked for
by the respective decoders, like fmt looks for the String method.
When the json unmarshaller does a dynamic interface check
to see if the object knows how to json unmarshal itself, it
had better not use the gob unmarshaller instead.

Russ

[cw, 2010-04-27 18:56:30]

On Tue, Apr 27, 2010 at 11:51:28AM -0700, Russ Cox wrote:

> When the json unmarshaller does a dynamic interface check
> to see if the object knows how to json unmarshal itself, it
> had better not use the gob unmarshaller instead.

ok, that makes perfect sense, i was thinking about it from the wrong
end

thanks!