Skip to content
This repository has been archived by the owner on May 12, 2020. It is now read-only.

Fix remove orphans #25

Merged
merged 2 commits into from Feb 24, 2016
Merged

Fix remove orphans #25

merged 2 commits into from Feb 24, 2016

Conversation

ghost
Copy link

@ghost ghost commented Feb 22, 2016

I investigated #24 and I found 2 problems:

  • When an orphan node was detected and removed, the indexes in the nodes vector changed for all the nodes after it, but the edges were not updated to match new indexes. It then triggered an out-of-bound panic on the next loop execution. Also, edges originating from the removed node were not removed.
  • The preliminary bound check was incorrect and did not remove edges pointing at/originating from 1-past the number of nodes, resulting in an out-of-bound panic in the next loop.

Vaelden added 2 commits February 22, 2016 22:06
The condition to remove it was index > len and should be index >= len
Also used standard library function Vec::retain
@yo-bot
Copy link
Collaborator

yo-bot commented Feb 22, 2016

Thanks for the pull request, and welcome! The team is excited to review your changes, and you should hear from @kbknapp (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. Due to 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 the contribution instructions for more information.

@kbknapp
Copy link
Owner

kbknapp commented Feb 24, 2016

thanks for this fix! 👍

@homu r+

@homu
Copy link
Collaborator

homu commented Feb 24, 2016

📌 Commit 1d1f60a has been approved by kbknapp

@homu homu merged commit 1d1f60a into kbknapp:master Feb 24, 2016
homu added a commit that referenced this pull request Feb 24, 2016
Fix remove orphans

I investigated #24 and I found 2 problems:

* When an orphan node was detected and removed, the indexes in the nodes vector changed for all the nodes after it, but the edges were not updated to match new indexes. It then triggered an out-of-bound panic on the next loop execution. Also, edges originating from the removed node were not removed.
* The preliminary bound check was incorrect and did not remove edges pointing at/originating from 1-past the number of nodes, resulting in an out-of-bound panic in the next loop.
@homu
Copy link
Collaborator

homu commented Feb 24, 2016

⚡ Test exempted - status

@amarant
Copy link

amarant commented Mar 23, 2016

Could you publish a new version with that change, because I have the same problem with installing from cargo-extras ?
Thanks.

@kbknapp
Copy link
Owner

kbknapp commented Mar 23, 2016

Ah yeah, sorry!

@kbknapp
Copy link
Owner

kbknapp commented Mar 23, 2016

@amarant v0.2.2 is out on crates.io

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants