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

peer: Rename MRU to LRU. #976

Merged
merged 3 commits into from
Feb 13, 2018
Merged

peer: Rename MRU to LRU. #976

merged 3 commits into from
Feb 13, 2018

Conversation

randomshinichi
Copy link
Contributor

Fixes #967

Copy link
Member

@davecgh davecgh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR.

I haven't really reviewed this in depth yet, but a couple of things right away:

  • It's failing the unit tests. That will need to be resolved.
  • Also, per the referenced issue, it should be lruInventoryCache and lruNonceCache as opposed to lruInventoryMap and lruNonceMap, respectively.

@randomshinichi
Copy link
Contributor Author

Made the changes. I even changed map -> cache in the comments, although I'm not sure that's entirely a good idea. A cache sounds too general - a map makes me think of a hash map, which would be one concrete way to implement a cache. Have a look and tell me what you think.

Copy link
Member

@davecgh davecgh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice. Thanks for the updates. The code looks good.

The last bits will be a few things related to the commits and then we can get this merged:

  • Please make the commits match the Model Git Commit Messages in the code contribution guidelines. Most notably to make sure each one has a prefix which describes the package it touches (they're all peer here).
  • Please do not include issue numbers in commits because they end up referring to the wrong issues when they get merged into other repos during syncs. Instead, issue number references should be limited to the git PR descriptions.
  • Squash the comment update commits into their respective rename commit. Ideally this PR would only be 2 commits (One for the MruInventoryMap to LruInventoryCache rename and updates, and a second one for the MruNonceMap to LruNonceCache rename and updates).

@davecgh davecgh changed the title Issue #967 MRU LRU rename peer: Rename MRU to LRU. Jan 28, 2018
@davecgh
Copy link
Member

davecgh commented Jan 29, 2018

Once the review comments are addresses we can get this merged. Feel free to pop into the dev channel on irc/slack/discord if you want to discuss.

The names mean that the caches contain the Most Recently Used entries.
But they behave in a Least Recently Used fashion.
It's a cache, but it is implemented with a map.
It's a cache, butt it is implemented with a map.
@randomshinichi
Copy link
Contributor Author

I tidied up the commits. Splitting them up per component would've taken too long, though. I think doing it this way makes logical sense too.

I might use this model git commit format for my code too.

Copy link
Member

@davecgh davecgh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the updates. Looks good.

@davecgh davecgh merged commit 516f300 into decred:master Feb 13, 2018
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