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

CacheKey constructor should take Request by reference #24353

Closed
jdm opened this issue Oct 3, 2019 · 5 comments
Closed

CacheKey constructor should take Request by reference #24353

jdm opened this issue Oct 3, 2019 · 5 comments

Comments

@jdm
Copy link
Member

@jdm jdm commented Oct 3, 2019

In CacheKey's new method in http_cache.rs it takes a Request argument by value, but then it calls a method on it that only needs a borrowed value. We clone request objects every time we call CacheKey::new, and this is wasteful.

Code: components/net/http_cache.rs

No need to run tests; if it builds, that's good enough for a pull request.

@highfive
Copy link

@highfive highfive commented Oct 3, 2019

Hi! If you have any questions regarding this issue, feel free to make a comment here, or ask it in the #servo channel in IRC.

If you intend to work on this issue, then add @highfive: assign me to your comment, and I'll assign this to you. 😄

@Tugdual
Copy link
Contributor

@Tugdual Tugdual commented Oct 3, 2019

@highfive: assign me

@highfive highfive added the C-assigned label Oct 3, 2019
@highfive
Copy link

@highfive highfive commented Oct 3, 2019

Hey @Tugdual! Thanks for your interest in working on this issue. It's now assigned to you!

@PeaceRebel
Copy link
Contributor

@PeaceRebel PeaceRebel commented Oct 3, 2019

@jdm , I'm new to rust. Instead of cloning, passing a reference would get it done, right?

@jdm
Copy link
Member Author

@jdm jdm commented Oct 3, 2019

@PeaceRebel Yes, that is what I suggested. It looks like @Tugdual is working on this, however.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

4 participants
You can’t perform that action at this time.