Skip to content

Conversation

@mgood
Copy link
Contributor

@mgood mgood commented Jan 16, 2018

The array encoder assumed that arrays had at least one value, so it
would serialize them with a zero-value for the array, such as [0].

This adds a test to reproduce the issue, and updates the encoder to
write an empty array if the length is 0.

The array encoder assumed that arrays had at least one value, so it
would serialize them with a zero-value for the array, such as `[0]`.

This adds a test to reproduce the issue, and updates the encoder to
write an empty array if the length is 0.
}

func (encoder *arrayEncoder) Encode(ptr unsafe.Pointer, stream *Stream) {
if encoder.arrayType.Len() == 0 {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looking at this again I realized that since an array's length is a static part of the type, I could also move this into encoderOfArray to return a special emptyArrayEncoder instead of checking the length on each value. That would be faster and also fix some assumptions in EncodeInterface and IsEmpty that the array has a non-zero length.

I don't quite understand the special optimization for interface{} in EncodeInterface and when that case would get called. The output in that case is hard-coded to produce [null], but it seems like that also ignores the original length of the array, even if the contents should all be null.

Copy link
Contributor

Choose a reason for hiding this comment

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

emptyArrayEncoder sounds like a good idea, can you make it happen?

@codecov
Copy link

codecov bot commented Jan 17, 2018

Codecov Report

Merging #225 into master will decrease coverage by 0.13%.
The diff coverage is 50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #225      +/-   ##
==========================================
- Coverage   83.86%   83.72%   -0.14%     
==========================================
  Files          42       42              
  Lines        5416     5426      +10     
==========================================
+ Hits         4542     4543       +1     
- Misses        690      697       +7     
- Partials      184      186       +2
Impacted Files Coverage Δ
feature_reflect_array.go 80.7% <50%> (-5.02%) ⬇️
feature_iter_object.go 53.43% <0%> (-2.65%) ⬇️
feature_reflect_slice.go 87.95% <0%> (+0.29%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c3ed5e8...807e4a8. Read the comment docs.

Instead of checking the array length in encode, this can be checked up
front in `encoderOfArray` since the array type has a fixed length
determined at compile time. So return an `emptyArrayEncoder` that simply
writes an empty array to the stream.
@taowen taowen merged commit e31252f into json-iterator:master Jan 23, 2018
zhenzou pushed a commit to zhenzou/jsoniter that referenced this pull request Feb 2, 2022
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.

2 participants