Skip to content

Use bytes for file field on attachment#1886

Closed
ImUrX wants to merge 2 commits intotwilight-rs:mainfrom
ImUrX:bytes
Closed

Use bytes for file field on attachment#1886
ImUrX wants to merge 2 commits intotwilight-rs:mainfrom
ImUrX:bytes

Conversation

@ImUrX
Copy link
Contributor

@ImUrX ImUrX commented Sep 2, 2022

The idea of using the Bytes struct comes from being able to have more flexibility when adding attachments.
Quoting the docs it mentions that

Bytes values facilitate zero-copy network programming by allowing multiple Bytes objects to point to the same underlying memory. This is managed by using a reference count to track when the memory is no longer needed and can be freed.

I would honestly just add an impl Into<Bytes> for the from_bytes function but it would stop being const so it would also require a breaking change.

@github-actions github-actions bot added the c-model Affects the model crate label Sep 2, 2022
@7596ff
Copy link
Contributor

7596ff commented Sep 2, 2022

Ideally the struct would take &'a [u8] but at the time there were too many lifetime issues that I could not sort out. I went with Vec because I needed to release the change, and it was a temporary fix that ended up lasting longer than it should have. Since Bytes is still the same amount of allocations when using borrowed data, I don't see this change being necessary at the moment.

@7596ff
Copy link
Contributor

7596ff commented Sep 7, 2022

Since this isn't a change we are going to be making soon, I'm going to give this a close.

@7596ff 7596ff closed this Sep 7, 2022
itohatweb added a commit that referenced this pull request Sep 17, 2022
Instead of taking `&'a [u8]` for attachments, which could cause lifetime
issues when using functions like `fn create_attachment<'a>() -> Attachment<'a>`, `Cow<'a, u8>` can be used to support both creating attachments from owned and borrowed bytes.

This is a followup idea to: #1886
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c-model Affects the model crate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants