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

Modify interface to take parameters as reference #31

Closed
ionut-arm opened this issue Jun 29, 2020 · 7 comments
Closed

Modify interface to take parameters as reference #31

ionut-arm opened this issue Jun 29, 2020 · 7 comments
Labels
enhancement New feature or request medium Effort label

Comments

@ionut-arm
Copy link
Member

Most parameters get owned by the client once a method call is performed - this could/should be changed so that most parameters are instead passed by reference. The client can clone whatever it needs inside the methods.

The question is whether data buffers that we would normally own and zeroize on drop should be passed as references too.

@ionut-arm ionut-arm added the enhancement New feature or request label Jun 29, 2020
@ionut-arm
Copy link
Member Author

@hug-dev would this impact the SE driver in a major way?

@hug-dev
Copy link
Member

hug-dev commented Jun 29, 2020

this could/should be changed so that most parameters are instead passed by reference.

what do you think would be the advantages of this? Do you have any examples in mind?

@hug-dev would this impact the SE driver in a major way?

I don't think so! It can adapt 😃

@ionut-arm
Copy link
Member Author

what do you think would be the advantages of this? Do you have any examples in mind?

Yeah, the key name is a big example - we take it as an owned string everywhere... it would be much nicer and idiomatic for a user to just do &key_name instead of key_name.clone() every time. But pretty much all other attributes (e.g. AsymmetricEncryption for sign and verify) do the same

@hug-dev
Copy link
Member

hug-dev commented Jun 29, 2020

Yeah, the key name is a big example - we take it as an owned string everywhere... it would be much nicer and idiomatic for a user to just do &key_name instead of key_name.clone() every time.

Definitely agree for the name! For the other non-Copy type, I guess we will have to review them on a case-by-case basis. Basically it only depends if the client needs to re-use the values after calling the function.

@hug-dev
Copy link
Member

hug-dev commented Dec 11, 2020

I guess the default should be to take references everywhere unless the types are Copy.
For String it should be &str and for Vec<u8> &[u8]

@ionut-arm
Copy link
Member Author

I think we've debated this before - passing by reference ends up "hiding" the fact that you actually want an instance of that data and that you're going to clone it. I don't know if it's sensible to just assume that if a type implements Clone we can treat a reference the same way as an owned instance.

@hug-dev hug-dev added the medium Effort label label Feb 3, 2021
@hug-dev
Copy link
Member

hug-dev commented Apr 29, 2021

I think most BasicClient methods take references to buffers now. The thing that would be nice is to use &str instead of String for keys as the latter implies making .clone everywhere... Ideally the clone should be done at the end to hide the complexity before.

Renaming this issue to that. Actually closing and opening a new one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request medium Effort label
Projects
None yet
Development

No branches or pull requests

2 participants