Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Prefer slice argument when passing in content key #165

Merged
merged 3 commits into from
Nov 1, 2021

Conversation

carver
Copy link
Collaborator

@carver carver commented Oct 28, 2021

This change probably looks a little surprising in isolation. Right now it causes an extra clone (because we were previously able to move in a vector that now has to be cloned inside the method).

Here is a super draft-y PR to help show the context of where I think it makes more sense:
https://github.com/ethereum/trin/pull/166/files#diff-fd7cee899607b5a0b4e30853e7bb9f8a191ed26926c7724f6b66d3070f62cf56R67-R72

Also, at some point, it should be possible to build the request from a slice without copying it first. It just doesn't seem like a priority now. But we might as well prepare the API to head that direction IMO. As always, open to discussion.

Copy link
Contributor

@lithp lithp left a comment

Choose a reason for hiding this comment

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

Definitely a bit surprising but looks good to me!

A pattern I've seen is to accept a generic Into<Vec<u8>> type, then call .into() on the passed argument. example.

This should work here because Vec<T> implements From<&'_ [T]>, and it gives callers the freedom to pass in whatever is most convenient for them without needing to do any manual conversions.

And since the specialization happens at compile time there's a good chance that the code only ends up doing as many copies as are necessary. (If a Vec is passed in then it's simply moved into the correct position in the new struct)

@carver
Copy link
Collaborator Author

carver commented Nov 1, 2021

Beautiful, yes, this looks like the best convention I've seen so far. I'll update to this before merging.

A pattern I've seen is to accept a generic Into<Vec<u8>> type, then call .into() on the passed argument. example.

@njgheorghita check out the example link above, it's a great way to minimize cloning and make it easier for the caller to use whichever type is more convenient.

@carver carver force-pushed the content-key-arg-as-slice branch 2 times, most recently from ad6b5d1 to 2e4be36 Compare November 1, 2021 18:56
Also, skip dependencies (although this seems to be buggy since CI still
reports "Checking" for all the dependencies)
@carver carver force-pushed the content-key-arg-as-slice branch from 2e4be36 to c4c3531 Compare November 1, 2021 20:01
@carver carver merged commit 6fa587b into master Nov 1, 2021
@carver carver deleted the content-key-arg-as-slice branch November 1, 2021 20:27
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants