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

Refactor to reuse functions and improve code coverage #531

Merged
merged 2 commits into from
May 5, 2024

Conversation

fxamacker
Copy link
Owner

This PR refactors code to reuse functions and improve code coverage:

  • Remove encodeFixedLengthStruct() and reuse encodeStruct()

    Previously, encodeStruct() used extra buffer to encode elements to get actual encoded element count. To avoid this overhead, encodeFixedLengthStruct() was created to encode fixed length struct (struct without any "omitempty" fields) since encoded element count is always known in this use case.

    Since @benluddy's PR Encode structs directly to output buffer. #519, encodeStruct() doesn't use extra buffer any more, and encodeFixedLengthStruct() isn't necessary. Thanks Ben! 👍

  • Combine encodeHead() and encodedHeadLen() to avoid these two functions out of sync.

    encodeHead() is modified to return encoded head length so caller doesn't need to call encodedHeadLen() separately.

This commit removes encodeFixedLengthStruct() and reuses
encodeStruct() to simplify code.

Previously, encodeStruct() used extra buffer to encode elements
to get actual encoded element count.  To avoid this overhead,
encodeFixedLengthStruct() was created to encode fixed
length struct (struct without any "omitempty" fields) since
encoded element count is always known in this use case.

With PR #519 (#519),
encodeStruct() doesn't use extra buffer any more, and
encodeFixedLengthStruct() isn't necessary.
@fxamacker fxamacker added the improvement improvement that does not add new feature label May 5, 2024
@fxamacker fxamacker self-assigned this May 5, 2024
@fxamacker
Copy link
Owner Author

@benluddy @x448 PTAL 🙏

Copy link
Contributor

@x448 x448 left a comment

Choose a reason for hiding this comment

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

👍

Add comment for encodeHead to say return value is number of bytes written.

This commit removes encodedHeadLen() and modifies encodeHead()
to return encoded head length to simplify code.
@fxamacker fxamacker force-pushed the fxamacker/refactor-to-reuse-code branch from af12989 to f83a7d0 Compare May 5, 2024 23:46
@fxamacker fxamacker merged commit 5a131e1 into master May 5, 2024
17 checks passed
@benluddy
Copy link
Contributor

benluddy commented May 6, 2024

Looks good!

@fxamacker fxamacker deleted the fxamacker/refactor-to-reuse-code branch September 7, 2024 20:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement improvement that does not add new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants