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

reduce the number of clones by introducing lifetime to rootless. #194

Merged
merged 2 commits into from
Aug 9, 2021

Conversation

utam0k
Copy link
Collaborator

@utam0k utam0k commented Aug 8, 2021

I'm not sure if I should name the lifetime parameters.

@utam0k utam0k requested a review from Furisto August 8, 2021 08:38
@Furisto
Copy link
Collaborator

Furisto commented Aug 8, 2021

I am not sure about this change. This would trade ergonomics (no need to ensure that lifetimes match up) for performance (less clones). Is the performance gain significant though?

@utam0k
Copy link
Collaborator Author

utam0k commented Aug 9, 2021

@Furisto
Thank you for your review. Performance gains are currently small. However, heavy use of clone() can be a performance problem. It is possible to introduce a lifetime after this problem surfaces, but that could be a major code change. I thought that we should introduce something simple. Of course, the code will be more or less complex, so there is a trade-off there. I'd love to hear your opinions on this.
This PR is a small step at the moment, but it is a step towards introducing order in the use of lifetime in youki's code.

@utam0k
Copy link
Collaborator Author

utam0k commented Aug 9, 2021

Of course, this is not to prohibit the use of clones, but to reduce the use of clones in simple situations. It is useful and should be used in situations where the code becomes unreasonably complex.

@Furisto Furisto merged commit f293c01 into youki-dev:main Aug 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants