Skip to content

fix: adding a self-loop should not add two edges (#183)#187

Open
dlyongemallo wants to merge 2 commits intozxcalc:masterfrom
dlyongemallo:fix_self_loop_duplication
Open

fix: adding a self-loop should not add two edges (#183)#187
dlyongemallo wants to merge 2 commits intozxcalc:masterfrom
dlyongemallo:fix_self_loop_duplication

Conversation

@dlyongemallo
Copy link

No description provided.

Copilot AI review requested due to automatic review settings November 9, 2025 08:38
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes a bug where self-loops (edges from a vertex to itself) were being added twice to the graph's internal data structure. The fix adds a check to prevent the duplicate addition of the reverse half-edge when source and target vertices are the same.

  • Added a check if s != t to prevent adding the reverse half-edge for self-loops in add_edge_with_type
  • Added a test case self_loop_not_duplicated to verify the fix
Comments suppressed due to low confidence (2)

quizx/src/vec_graph.rs:250

  • The remove_edge method will incorrectly remove self-loops twice. When s == t, both remove_half_edge(s, t) and remove_half_edge(t, s) will attempt to remove the same edge from the adjacency list, but the second call will fail to find it (since it was already removed). While this may not cause a crash, it's inconsistent with the add_edge_with_type fix. Consider adding a similar if s != t check here to only call remove_half_edge(t, s) when the vertices differ.
    fn remove_edge(&mut self, s: V, t: V) {
        self.nume -= 1;
        self.remove_half_edge(s, t);
        self.remove_half_edge(t, s);
    }

quizx/src/vec_graph.rs:279

  • The set_edge_type method has the same issue with self-loops as the original bug in add_edge_with_type. When s == t, the method will update the same edge twice (lines 268 and 275), which is unnecessary and could cause issues if the implementation changes. Consider adding an if s != t check before the second update block (lines 273-278) to match the fix in add_edge_with_type.
    fn set_edge_type(&mut self, s: V, t: V, ety: EType) {
        if let Some(Some(nhd)) = self.edata.get_mut(s) {
            let i = Graph::index(nhd, t).expect("Edge not found");
            nhd[i] = (t, ety);
        } else {
            panic!("Source vertex not found");
        }

        if let Some(Some(nhd)) = self.edata.get_mut(t) {
            let i = Graph::index(nhd, s).expect("Edge not found");
            nhd[i] = (s, ety);
        } else {
            panic!("Target vertex not found");
        }
    }

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@dlyongemallo dlyongemallo force-pushed the fix_self_loop_duplication branch from f22db29 to bee0bee Compare February 1, 2026 20:40
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.

1 participant