docs: comments for blob, stuffer methods#5326
Conversation
docs/DEVELOPMENT-GUIDE.md
Outdated
| ### s2n_blob: keeping track of memory ranges | ||
|
|
||
| `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. |
There was a problem hiding this comment.
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"?
There was a problem hiding this comment.
Updated to add an explicit "either ... or" and "non-owned slice of memory".
s2n_blobis a data structure representing either allocated, "owned" memory or a reference to some other non-owned slice of memory.
I know it's a little bit wordy but I prefer that over "slice of memory", because I want to stress that blobs have two semantically different use cases and if you confuse them it will blow up in your face 🙃
| * Resize a blob to `size`. The blob must be allocated or empty. | ||
| * |
There was a problem hiding this comment.
Did you mean "growable"?
| * Resize a blob to `size`. The blob must be allocated or empty. | |
| * | |
| * Resize a blob to `size`. The blob must be growable or empty. | |
| * |
There was a problem hiding this comment.
Nope, I did mean "allocated". This was a concious decision because I always have to mentally wrangle with the meaning of "growable" everytime I see it.
Lines 40 to 48 in e8042c0
Mentally my main distinction in blobs is whether they are allocated/owned or references.
The concept of "growable" confuses me because I then expect it to be an additional quality outside of the owned/reference distinction. Basically it makes me think that blobs support "allocated, non-growable" blobs. But that is incorrect, and aside from a weird 0-size edge case, "is it growable" is just "is it allocated".
Lmk if you still prefer the "growable" term and I'll go ahead and update it.
|
This PR has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
This PR has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
stuffer/s2n_stuffer.c
Outdated
| } | ||
|
|
||
| /** | ||
| * Copy the internal data of `stuffer` to `out`. |
There was a problem hiding this comment.
This only copies from the read cursor to the write cursor, not sure if that is helpful point to make.
There was a problem hiding this comment.
huh, I someone missed that this one already has a comment in the header file. We really need to consolidate our comments into one or the other 🥲
Removing this comment for now (since it's already in the header file)
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.