Skip to content
This repository was archived by the owner on Nov 6, 2020. It is now read-only.

Fixed an issue with forked counters #363

Merged
merged 6 commits into from
Feb 7, 2016
Merged

Fixed an issue with forked counters #363

merged 6 commits into from
Feb 7, 2016

Conversation

arkpar
Copy link
Collaborator

@arkpar arkpar commented Feb 5, 2016

No description provided.

@arkpar arkpar added the A0-pleasereview 🤓 Pull request needs code review. label Feb 5, 2016
@gavofyork gavofyork added A8-looksgood 🦄 Pull request is reviewed well. A0-pleasereview 🤓 Pull request needs code review. and removed A0-pleasereview 🤓 Pull request needs code review. A8-looksgood 🦄 Pull request is reviewed well. labels Feb 5, 2016
@gavofyork
Copy link
Contributor

uncomfortable with quite a few of the changes here.

@arkpar arkpar added the A4-gotissues 💥 Pull request is reviewed and has significant issues which must be addressed. label Feb 7, 2016
@arkpar arkpar removed the A4-gotissues 💥 Pull request is reviewed and has significant issues which must be addressed. label Feb 7, 2016
index += 1;
}

let canon_inserts = canon_inserts.drain(..).collect::<HashSet<_>>();
// Purge removed keys if they are not referenced and not re-inserted in the canon commit
for h in to_remove.iter().filter(|h| !counters.contains_key(h) && !canon_inserts.contains(h)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

.contains -> O(n)

better to use a hash_set for canon_inserts?

@gavofyork gavofyork added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Feb 7, 2016
gavofyork pushed a commit that referenced this pull request Feb 7, 2016
Fixed an issue with forked counters
@gavofyork gavofyork merged commit c55ff79 into master Feb 7, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A8-looksgood 🦄 Pull request is reviewed well.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants