Skip to content

docs: comments for blob, stuffer methods #5326

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jmayclin
Copy link
Contributor

Description of changes:

Add comments for some stuffer and blob methods.

I have reread the implementations of a lot of stuffer/blob methods many times because I keep forgetting what they do, and can't tell from the name.

I acknowledge that most of the methods are small so reading them shouldn't be a big deal, but having to jump in and out of the files when I am reviewing PR's is not fun. By adding comments, my IDE will just tell me what the functions do when I mouse over them.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@jmayclin jmayclin requested review from goatgoose and lrstewart May 22, 2025 00:42
@github-actions github-actions bot added the s2n-core team label May 22, 2025
@jmayclin jmayclin marked this pull request as ready for review May 22, 2025 02:14
`s2n_blob` is a very simple data structure:
`s2n_blob` is a data structure representing allocated, "owned" memory or a reference to some other slice of memory.
Copy link
Contributor

Choose a reason for hiding this comment

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

I know you later say "or a reference to some other slice of memory", but I worry that someone is still still going to read this as "blobs are always allocated". Maybe just "s2n_blob is a data structure representing a slice of memory"?

Comment on lines +63 to +64
/**
* Initialize a stuffer to reference some mutable data `in`.
Copy link
Contributor

Choose a reason for hiding this comment

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

I know the function signature specifies that the data is mutable, but often we just assume it's not because there's no immutable version 😬 I wonder if you should remove the "mutable".

Same for blob down below.

Comment on lines +200 to +201
* Resize a blob to `size`. The blob must be allocated or empty.
*
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you mean "growable"?

Suggested change
* Resize a blob to `size`. The blob must be allocated or empty.
*
* Resize a blob to `size`. The blob must be growable or empty.
*

Comment on lines +268 to +269
* Copy the data in `from` to a new allocated blob.
*/
Copy link
Contributor

@lrstewart lrstewart May 27, 2025

Choose a reason for hiding this comment

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

This one always trips me up. I wonder if we should be even more specific about the fact that to will be allocated as part of this operation. "Allocate enough memory for to to contain all the data in from, then copy the data in from to to."

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants