code review 152270044: encoding/json: fix handling of null with ,string fields

encoding/json: fix handling of null with ,string fields

Fixes issue 8587.

[rsc, 2014-10-07 00:11:54]

Hello golang-codereviews@googlegroups.com (cc: bradfitz, iant, r),

I'd like you to review this change to
https://code.google.com/p/go/

[bradfitz, 2014-10-07 14:00:09]

https://codereview.appspot.com/152270044/diff/40001/src/encoding/json/decode_...
File src/encoding/json/decode_test.go (right):

https://codereview.appspot.com/152270044/diff/40001/src/encoding/json/decode_...
src/encoding/json/decode_test.go:1073: // Test that a null for ,string is not
replaced with the previous quoted string (issue 7046).
Why is this test changing so much? Can't you add a new test instead to make it
clear that behavior isn't being changed? (if it's not)

If not, and you intend for the old behavior to change, the CL description
doesn't explain how or why or when the change being reverted was introduced.

[rsc, 2014-10-07 14:23:54]

https://codereview.appspot.com/152270044/diff/40001/src/encoding/json/decode_...
File src/encoding/json/decode_test.go (right):

https://codereview.appspot.com/152270044/diff/40001/src/encoding/json/decode_...
src/encoding/json/decode_test.go:1073: // Test that a null for ,string is not
replaced with the previous quoted string (issue 7046).
On 2014/10/07 14:00:08, bradfitz wrote:
> Why is this test changing so much? Can't you add a new test instead to make it
> clear that behavior isn't being changed? (if it's not)
> 
> If not, and you intend for the old behavior to change, the CL description
> doesn't explain how or why or when the change being reverted was introduced.

I put that right here, in this comment. I also updated issue 7046 with an
explanation.

[bradfitz, 2014-10-07 14:29:41]

LGTM

Okay,  I read the comment on 7046. I still feel like this test is borderline
testing too many things at once, but it's fine.

[rsc, 2014-10-07 15:06:19]

On Tue, Oct 7, 2014 at 10:29 AM, <bradfitz@golang.org> wrote:

> LGTM
>
> Okay,  I read the comment on 7046. I still feel like this test is
> borderline testing too many things at once, but it's fine.
>

The test broke due to this change. I had to update it. Now it happens to be
testing that there's no error as well as the right results, but it's a bit
hard to disentangle those.

[rsc, 2014-10-07 15:07:09]

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

encoding/json: fix handling of null with ,string fields

Fixes issue 8587.

LGTM=bradfitz
R=golang-codereviews, bradfitz
CC=golang-codereviews, iant, r
https://codereview.appspot.com/152270044

[gobot, 2014-10-07 16:57:54]

This changed caused perf changes on windows-amd64-perf:


json-1                    old          new      delta
time                173976868    178661092      +2.69

json-4                    old          new      delta
time                 45385925     46501931      +2.46

json-8                    old          new      delta
time                 24273888     24810371      +2.21

http://build.golang.org/perfdetail?commit=620024295679c290fa100160d7e5ea8693b...



—gobot