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

implement signed integer encoding/decoding #185

Merged
merged 6 commits into from
Feb 23, 2021

Conversation

fbegyn
Copy link
Contributor

@fbegyn fbegyn commented Feb 20, 2021

Resolves #184 by implementing the signed integer encoders and decoders.

The int(uint( is not necessarily pretty, but the other options to my knowledge are:

  • unsafe pointer casting (like in nlenc)
  • manually parsing the bytes (but then endianess is a thing)

@mdlayher
Copy link
Owner

Thanks for this! I will try to take a look later today.

Copy link
Owner

@mdlayher mdlayher left a comment

Choose a reason for hiding this comment

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

Overall seems fine. I would prefer not to add any new API to nlenc though: it is effectively deprecated since the addition of the attribute encoder/decoder types. Maybe I should officially add a deprecated tag on the package comment.

attribute.go Outdated
}

b := make([]byte, 2)
ae.ByteOrder.PutUint16(b,uint16(v))
Copy link
Owner

Choose a reason for hiding this comment

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

I don't think this is gofmt'd.

@@ -909,18 +983,18 @@ func TestAttributeEncoderOK(t *testing.T) {
},
},
{
name: "uint native endian",
name: "uint-int32 native endian",
Copy link
Owner

Choose a reason for hiding this comment

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

Just "int" for all these cases, not int32.

Seems like my auto gofmt didn't trigger on some parts.
As the `nlenc` module is deprecated, new tests and features should not be
introduced to it.
@fbegyn
Copy link
Contributor Author

fbegyn commented Feb 23, 2021

With the nlenc being deprecated, I stripped out most of the code and tests in there, I only let the put signed 32 test in there since that function is already part of the module. I've also ran gofmt against all the files again, looks like my last save didn't autocall it for some reason.

Copy link
Owner

@mdlayher mdlayher left a comment

Choose a reason for hiding this comment

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

LGTM!

@mdlayher mdlayher merged commit 0b57953 into mdlayher:master Feb 23, 2021
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.

add support for signed integers encoding/decoding
2 participants