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

Assotiative cache implementation #71

Merged
merged 20 commits into from
Oct 31, 2023
Merged

Assotiative cache implementation #71

merged 20 commits into from
Oct 31, 2023

Conversation

ivan-egorov42
Copy link
Contributor

Tests will be added soon. Pls, have a look @mmamayka

Copy link
Member

@mmamayka mmamayka left a comment

Choose a reason for hiding this comment

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

Looks good, but need to be polished

include/besm-666/util/assotiative-cache.hpp Outdated Show resolved Hide resolved
include/besm-666/util/assotiative-cache.hpp Outdated Show resolved Hide resolved
include/besm-666/util/assotiative-cache.hpp Outdated Show resolved Hide resolved
include/besm-666/util/assotiative-cache.hpp Outdated Show resolved Hide resolved
include/besm-666/util/assotiative-cache.hpp Outdated Show resolved Hide resolved
include/besm-666/util/assotiative-cache.hpp Outdated Show resolved Hide resolved
include/besm-666/util/assotiative-cache.hpp Outdated Show resolved Hide resolved
include/besm-666/util/assotiative-cache.hpp Outdated Show resolved Hide resolved
include/besm-666/util/assotiative-cache.hpp Show resolved Hide resolved
include/besm-666/util/assotiative-cache.hpp Outdated Show resolved Hide resolved
@ivan-egorov42
Copy link
Contributor Author

@mmamayka Pls, have a look.

Copy link
Member

@mmamayka mmamayka left a comment

Choose a reason for hiding this comment

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

Looks really good! But unit tests need to be added. At least one dummy unit test that checks, the Cache class can be properly instantiated, cache entry can be added and found after. Also, I propose to check a cache replacement correctness by adding N elements with same hash to the cache, where N >= Sets, and looking for them.

@ivan-egorov42
Copy link
Contributor Author

@mmamayka pls, have a look.

mmamayka
mmamayka previously approved these changes Oct 22, 2023
Copy link
Member

@mmamayka mmamayka left a comment

Choose a reason for hiding this comment

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

Cool!

levBagryansky
levBagryansky previously approved these changes Oct 24, 2023
Copy link
Contributor

@levBagryansky levBagryansky left a comment

Choose a reason for hiding this comment

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

@ivan-egorov42 LGTM! please just solve the edit above

unit_test/util/assotiative-cache-tests.cpp Outdated Show resolved Hide resolved
@ivan-egorov42 ivan-egorov42 removed the request for review from c71n93 October 31, 2023 15:38
Copy link
Contributor

@levBagryansky levBagryansky left a comment

Choose a reason for hiding this comment

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

@ivan-egorov42 ivan-egorov42 merged commit def6edd into main Oct 31, 2023
9 checks passed
@0pdd
Copy link
Collaborator

0pdd commented Oct 31, 2023

@ivan-egorov42 the puzzle #77 is still not solved.

@ivan-egorov42 ivan-egorov42 deleted the assotiative-cache branch October 31, 2023 19:22
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.

4 participants