-
Notifications
You must be signed in to change notification settings - Fork 232
Proposed Casher struct is in directory proposed_edition #18
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! I made a few requests.
src/proposed_addition/casher.rs
Outdated
} | ||
} | ||
|
||
pub fn value(&mut self, arg: U) -> V { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be possible to do a single lookup using the Entry
API:
self.values.entry(arg).or_insert_with_key(self.calculation)
Note that for this to work, the version number in .travis.yml
will have to be updated to 1.50.
calculation
will then take &U
, so we would no longer need U: Copy
. If you don't want it to take &U
, here's an alternative implementation:
self.values.entry(arg).or_insert_with_key(|&key| self.calculation(key))
These are all great edits! I'll get to them when I get back to my computer late tonight, I was running out the door when I submitted this! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm if you'd still like to test speed, you might be able to enhance the test that you made before: reduce the timer to 1 second or less, but do the insertion thousands of times. I suspect this version with or_insert
would fail.
src/cashing.rs
Outdated
/// let sixteen = squared.on(4); | ||
/// ``` | ||
pub fn on(&mut self, arg: U) -> V { | ||
// Clippy doesn't like that this isn't lazy but I'm fine with it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think clippy
complains because you'll be doing the computation even if the result is already cached, since arguments are always evaluated before a function (e.g., or_insert
) is called.
Did you have any luck with or_insert_with_key
? It takes a closure, which doesn't actually perform the computation unless called.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I ran into borrow problems since we take a mutable reference to self. I'll retry implementing it and if I can't make it work I'll follow up here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So after fighting for a bit, I didn't really run into anything I would consider a clean readable solution. I am pushing with a solution that doesn't use the entry api. It's a little longer, but I think very readable. I'm as in love with the entry api as the next guy, so let me know if this isn't acceptable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that case, it's good as is. I might give Entry
another try later :)
src/cashing.rs
Outdated
/// reads from a file and | ||
/// you think the contents of that file have changed even though the name | ||
/// has not. | ||
pub fn call_and_replace(&mut self, arg: U) -> V { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm this might be useful for a small number of users. In the name of conciseness, I'm not sure yet if I'd keep this feature in the long-term, since simple problems like those in competitions shouldn't have this kind of state-dependence. Anyway, for now I'll keep it however you decide.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even if it doesn't make sense for use in competition, I think this function makes a lot of sense for the structure. I can see it being used with rand in clever ways as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! Thank you for neatly documenting everything as well!
Let me know what you think about adding this sort of small patterns material!