Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

inception: Reset undefined fields when reusing objects. #195

Merged
merged 1 commit into from Dec 13, 2016

Conversation

cmeon
Copy link

@cmeon cmeon commented Oct 14, 2016

When reusing objects with sync.Pool, for example, and unmarshaling
objects, this resets the old fields, which are not defined in the
current JSON to their Zero values.

For example:

type M struct {
     Y []int
}

type A struct {
     I int
 T *M
}

type B struct {
     X int
     Z *A    `json:"Z,omitempty"`
     Y int   `json:"Y,omitempty"`
     K []int `json:"K,omitempty"`
}

buf0 = []byte(`{"X":1,"Z":{"I":3, "T":[3,4]},"Y":4,"K":[1,2,3]}`)
buf1 = []byte(`{"X":1,"K":[1,2,3]}`)

x0 := B{}
x0.UnmarshalJSON(buf0)
x1 := x0
x1.UnmarshalJSON(buf1)
  • This ensures that x0 != x1 by setting the fields in x1.Z = nil and x1.Y = 0
    after marshalling all JSON objects.

{{with $fieldName := $field.Name | printf "uj.%s"}}
{{if eq $field.Pointer true}}
{{$fieldName}} = nil
{{else if eq $field.Typ.Kind ` + strconv.FormatUint(uint64(reflect.Interface), 10) + `}}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All of this should be properly indented with tabs so the resulting code doesn't change with gofmt (like the other templates here).

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While nice, it doesn't matter too much, the generated code is all ran through fmt at the end, so the templates don't need to be whitespace perfect

@erikdubbelboer
Copy link
Contributor

Looks very useful and would surely prevent bugs.

I think it should get some test cases?

@erikdubbelboer
Copy link
Contributor

@pquerna did you have a look at this?

@erikdubbelboer
Copy link
Contributor

@pquerna I just updated the pull request with code that hides this feature behind a command line flag. This is needed for ffjson to continue being a drop in replacement for the normal json.

I also improved the test case and squashed all commits into one. I also did some benchmarking and it doesn't affect the performance at all, which is expected as it's only a couple of if statements.

I think you can really merge this now so other people can also use it.

@erikdubbelboer
Copy link
Contributor

Oh and I also fixed make clean and made sure make test rebuilds all the files as otherwise the tests might be using old generated files.

@erikdubbelboer
Copy link
Contributor

@pquerna is this project dead?

@pquerna pquerna merged commit c24ab9b into pquerna:master Dec 13, 2016
@erikdubbelboer
Copy link
Contributor

Thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants