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

Update ei_x_encode_long documentation to indicate variable length encoding. #8138

Merged
merged 1 commit into from
Mar 4, 2024

Conversation

mpope9
Copy link
Contributor

@mpope9 mpope9 commented Feb 16, 2024

The ei_x_encode_long function does not encode the passed value to the fixed length of a long. Updating the documentation to make that more obvious.

@CLAassistant
Copy link

CLAassistant commented Feb 16, 2024

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

CT Test Results

Tests are running... https://github.com/erlang/otp/actions/runs/7935778611

Results for commit 463f918

To speed up review, make sure that you have read Contributing to Erlang/OTP and that all checks pass.

See the TESTING and DEVELOPMENT HowTo guides for details about how to run test locally.

Artifacts

  • No CT logs found
  • No HTML docs found
  • No Windows Installer found

// Erlang/OTP Github Action Bot

@IngelaAndin IngelaAndin added the team:VM Assigned to OTP team VM label Feb 19, 2024
@jhogberg jhogberg self-assigned this Feb 19, 2024
@jhogberg
Copy link
Contributor

Thanks for the PR! I think this note would be better in the module description, as it makes it clearer that we don't promise that any of these functions encode to a fixed length.

@mpope9
Copy link
Contributor Author

mpope9 commented Feb 20, 2024

Thanks! That makes sense, will make that change.

Comment on lines 61 to 62
[`ei_x_buff`](ei.md#ei_x_buff). The `ei_x_` encode functions do not guarantee
the final buffer length, expect variable length buffers.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think belongs more in the paragraph above. How about something like?

All encode functions assume that the `buf` and `index` parameters point to a
buffer large enough for the data. Note that the binary term format uses variable-
length encoding so different values can require a different amount of space. For
example, smaller integer values can be more compact than larger ones. To get
the size of an encoded term, without encoding it, pass `NULL` instead of a
buffer pointer. Parameter `index` is incremented, but nothing will be encoded.
This is the way in `ei` to "preflight" term encoding.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it is much more clear. I didn't want to include the integer example directly incase it changes in the future but it is a good example. Will make this change.

Copy link
Contributor

@jhogberg jhogberg Feb 26, 2024

Choose a reason for hiding this comment

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

Great, I think the bit about "The ei_x_ encode functions do not guarantee
the final buffer length, expect variable length buffers." is a bit redundant given the paragraph above. Please remove that and squash the commits, and then I'll merge this straight away. :-)

@jhogberg jhogberg merged commit 6c1375c into erlang:master Mar 4, 2024
1 check passed
@jhogberg
Copy link
Contributor

jhogberg commented Mar 4, 2024

Merged, thanks again for the PR!

@mpope9 mpope9 deleted the mpope9-patch-1 branch March 4, 2024 15:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team:VM Assigned to OTP team VM
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants