search

maidsafe / lru_time_cache

LRU cache settable via size or time to live
BSD 3-Clause "New" or "Revised" License
104 stars 46 forks source link

Api additions #17

Closed inetic closed 9 years ago

inetic commented 9 years ago

Shouldn't break existing API as these are just added functions and comments where breaking changes should happen in the future.

Review on Reviewable

maidsafe-highfive commented 9 years ago

Thanks for the pull request, and welcome! The MaidSafe team is excited to review your changes, and you should hear from @chandraprakash (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. The way Github handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see CONTRIBUTOR.md for more information.

coveralls commented 9 years ago

Coverage Status

Coverage remained the same at 100.0% when pulling 5b97a1357974017ff24ea69a919ca1a72c9ea11f on inetic:api-changes into 669b08daed3f04411dba32bb19fb0fc7043c430c on maidsafe:master.

dirvine commented 9 years ago

r? @dirvine

dirvine commented 9 years ago

Review status: 0 of 1 files reviewed, 5 unresolved discussions, all commit checks successful.

src/lib.rs, line 103 [r1] (raw file): Agreed. We should do a breaking change soon. Can you co-ordinate with Ben to get this arranged in a task perhaps. Nice if it fitted an existing task.

src/lib.rs, line 147 [r1] (raw file): Agreed

src/lib.rs, line 152 [r1] (raw file): Nice we use len() here, I see we still use size() in some parts of the code in routing etc. we should also make sure to switch to len() there to

src/lib.rs, line 170 [r1] (raw file): Cool, one of the few times it's OK to leave commented code I think. Nice one

src/lib.rs, line 207 [r1] (raw file): Is this safe here and why ? (not clear to me) I wonder if the return should be a Result

Comments from the review on Reviewable.io

inetic commented 9 years ago

Review status: 0 of 1 files reviewed, 1 unresolved discussion, all commit checks successful.

src/lib.rs, line 207 [r1] (raw file): It is safe to unwrap the result because we know it is in because we just added it there. Though, it's a bit inefficient as we do twice lookup into the self.map variable (once through add and once through get_mut) while we probably could do just one. I'll check how it can be improved.

Comments from the review on Reviewable.io

inetic commented 9 years ago

Review status: 0 of 1 files reviewed, 1 unresolved discussion, all commit checks successful.

src/lib.rs, line 207 [r1] (raw file): I'm having a second look at the add function. It seems to be a little different from what one would expect from the Map::insert function. That is, add doesn't insert the new value into the case if the key was already present. I wonder whether this is intended? The standard Map::insert function replaces the value if one was already there.

Comments from the review on Reviewable.io

coveralls commented 9 years ago

Coverage Status

Coverage remained the same at 100.0% when pulling 01343e7a2dca4cea21bde2764b79f0bf835d692e on inetic:api-changes into 669b08daed3f04411dba32bb19fb0fc7043c430c on maidsafe:master.

inetic commented 9 years ago

Review status: 0 of 1 files reviewed, 2 unresolved discussions, some commit checks pending.

src/lib.rs, line 207 [r1] (raw file): OK, I think in terms of efficiency this is as good as the current Rust's API will allow us because we can't insert into a RBtreeMap and get the reference of the inserted element at the same time (as far as I know). The add function always add the key and value into the cache in this case because we're int the VacantEntry object, that means there wasn't an entry with the given key before which would prevent the add function to do the insertion.

Comments from the review on Reviewable.io

inetic commented 9 years ago

Review status: 0 of 1 files reviewed, 2 unresolved discussions, some commit checks pending.

src/lib.rs, line 129 [r2] (raw file): I now believe that the old add function was incorrect in two regards: the first, that it did not mark the key being added as 'recently used' and second that it didn't insert the new value into the cache if an entry with the same key was already there (contrary to what one would expect from a Map collection, see docs for RBtreeMap or HashMap).

Comments from the review on Reviewable.io

inetic commented 9 years ago

Review status: 0 of 1 files reviewed, 2 unresolved discussions, some commit checks pending.

src/lib.rs, line 129 [r2] (raw file): Ah, by "it did not mark the key being added as 'recently used' " I meant "it did not mark the key being added as 'recently used' if it was already there".

Comments from the review on Reviewable.io

benjaminbollen commented 9 years ago

Review status: 0 of 1 files reviewed, 4 unresolved discussions, all commit checks successful.

src/lib.rs, line 51 [r1] (raw file): I appreciate the Entry pattern, +1

src/lib.rs, line 103 [r1] (raw file): Can we duplicate these methods and deprecate the old names?

Comments from the review on Reviewable.io

benjaminbollen commented 9 years ago

Review status: 0 of 1 files reviewed, 4 unresolved discussions, all commit checks successful.

src/lib.rs, line 129 [r2] (raw file): Yes, I raised this issue, so it would be good to address this at the same time. It is an important behaviourial change to LRU cache though; should be handled with care

Comments from the review on Reviewable.io

dirvine commented 9 years ago

Review status: 0 of 1 files reviewed, 3 unresolved discussions, all commit checks successful.

src/lib.rs, line 129 [r2] (raw file): I think I would now go for full upheaval here and we bite the bullet. We can upgrade this crate in crates.io and when libs are ready they can switch.

Comments from the review on Reviewable.io

dirvine commented 9 years ago

Review status: 0 of 1 files reviewed, 3 unresolved discussions, all commit checks successful.

src/lib.rs, line 221 [r1] (raw file): OK sounds good enough for me then :+1:

Comments from the review on Reviewable.io

dirvine commented 9 years ago

Yes I think this is pretty much what this should look like. I would break the API and update the Cargo toml version and we can push this to crates.

Review status: 0 of 1 files reviewed, 3 unresolved discussions, all commit checks successful.

Comments from the review on Reviewable.io

inetic commented 9 years ago

Review status: 0 of 1 files reviewed, 3 unresolved discussions, all commit checks successful.

src/lib.rs, line 103 [r1] (raw file): Maybe I'm misunderstanding, but I think what you're suggesting is what I've done here. I left the add function intact so to not break anything, then created the new insert function (wasn't there before), which is more alike to the standard Map::insert functions.

Comments from the review on Reviewable.io

inetic commented 9 years ago

Review status: 0 of 1 files reviewed, 3 unresolved discussions, all commit checks successful.

src/lib.rs, line 129 [r2] (raw file): I'll need this changes in the Sentinel. But so far there is no API nor behavioral change as I left the old add function intact and added the new inert function. So we can slowly remove the add functions from the rest of the code and once done we can remove it here too.

Comments from the review on Reviewable.io

dirvine commented 9 years ago

All right Peter, nice one, here goes merge.