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

WIP: add IndexList::move_to_last to maintain ListIndex #4

Closed
wants to merge 1 commit into from

Conversation

jeffwashington
Copy link

@jeffwashington jeffwashington commented Jul 8, 2023

This is a work in progress. I will need help discussing if you require something more than this. I need to write a doc example if you find the basics of this PR acceptable. I can also write unit tests for this method. I am holding off on those until I get a preliminary review. Thank you.

There is value for the API to allow an entry in the list to be moved to the end of the linked list while maintaining the same index.
An example use case is implementing an LRU algorithm.
When an entry is found in a separate HashMap, we need to move the corresponding LRU queue entry to the end of the linked list. When necessary, we evict from the head of the linked list.

The current implementation requires a call to:

list.remove(index)
new_index = list.insert_last(key)

This requires a mutable reference to the entry in the HashMap which holds the index in the linked list.

This new method, move_to_last(index) updates the linked list such that the entry at index is now the last entry in the linked list. The index of the entry remains the same.

Copy link
Owner

@Fairglow Fairglow left a comment

Choose a reason for hiding this comment

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

Yes, the basics are acceptable and the change is beneficial. Just add an example as you noted, plus a test where the functionality is tested. The test should make sure that the correct behavior is achieved, i.e. it is not enough to simply call the function.

@jeffwashington
Copy link
Author

I appreciate the feedback and look. At this point, this doesn't have the benefit to justify the cost due to code around our usage changing. We may revisit in the future.

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