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

fix: use value receivers for MarshalXXX methods #117

Merged
merged 1 commit into from
Jul 3, 2024
Merged

Conversation

setrofim
Copy link
Contributor

@setrofim setrofim commented Jul 3, 2024

Use value, rather than pointer, receivers for MarshalCBOR and MarshalJSON methods. Since those methods don't mutate the structures, pointers are not necessary, and using them prevents the methods from being invoked when marshaling an instance of that structure (rather than a pointer to one). This is not a huge deal as CoRIM fields typically use pointers, however it can lead to surprising results when playing with the structures on their own.

Use value, rather than pointer, receivers for MarshalCBOR and
MarshalJSON methods. Since those methods don't mutate the structures,
pointers are not necessary, and using them prevents the methods from
being invoked when marshaling an instance of that structure (rather than
a pointer to one). This is not a huge deal as CoRIM fields typically use
pointers, however it can lead to surprising results when playing with
the structures on their own.

Signed-off-by: Sergei Trofimov <[email protected]>
Copy link
Contributor

@thomas-fossati thomas-fossati left a comment

Choose a reason for hiding this comment

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

LGTM

@thomas-fossati thomas-fossati merged commit 4e7323c into main Jul 3, 2024
9 checks passed
@thomas-fossati thomas-fossati deleted the marshal-fix branch July 3, 2024 16:13
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