-
Notifications
You must be signed in to change notification settings - Fork 10.9k
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
Make Graph.adjacentNodes
more transparent when a node gets deleted.
#6553
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -24,6 +24,7 @@ | |
import static com.google.common.graph.Graphs.checkPositive; | ||
import static java.util.Objects.requireNonNull; | ||
|
||
import com.google.common.collect.ImmutableSet; | ||
import com.google.errorprone.annotations.CanIgnoreReturnValue; | ||
import javax.annotation.CheckForNull; | ||
|
||
|
@@ -136,17 +137,23 @@ public boolean removeNode(N node) { | |
} | ||
} | ||
|
||
for (N successor : connections.successors()) { | ||
// Since views are returned, we need to copy the successors that will be removed. | ||
// Thus we avoid modifying the underlying view while iterating over it. | ||
for (N successor : ImmutableSet.copyOf(connections.successors())) { | ||
// requireNonNull is safe because the node is a successor. | ||
requireNonNull(nodeConnections.getWithoutCaching(successor)).removePredecessor(node); | ||
requireNonNull(connections.removeSuccessor(successor)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's been a while since I've looked at this code, but I'm a bit suspicious of the three different ways in which we're now accessing the connections of a node in this method:
We should (a) validate that we want those three different mechanisms, or use only the ones we actually want, and (b) document which one(s) we do want, and why. There's a lot of clever stuff going on in this code, which makes it easy to make mistakes in updating it. (To be clear, I think you've explained why you want the copy of the set in the places that you've made it, but the reason why we're using each of the above two mechanisms in different places is less clear.) |
||
--edgeCount; | ||
} | ||
if (isDirected()) { // In undirected graphs, the successor and predecessor sets are equal. | ||
for (N predecessor : connections.predecessors()) { | ||
// Since views are returned, we need to copy the predecessors that will be removed. | ||
// Thus we avoid modifying the underlying view while iterating over it. | ||
for (N predecessor : ImmutableSet.copyOf(connections.predecessors())) { | ||
// requireNonNull is safe because the node is a predecessor. | ||
checkState( | ||
requireNonNull(nodeConnections.getWithoutCaching(predecessor)).removeSuccessor(node) | ||
!= null); | ||
connections.removePredecessor(predecessor); | ||
--edgeCount; | ||
} | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why
ImmutableSet
here and notImmutableList
? Seems unnecessary to deduplicate at this point.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well,
connection.successors()
returns withSet<N>
, so I would not expect further deduplication at this point. I would be though concerned about the possible reordering what could happen through a hash-based set. (It might not cause issue during this cleanup, but it is not that trivial to prove.)Anyway I borrowed this approach from
StandardMutableNetwork.removeNode
where elements are indeed copied toImmutableList
, probably I just unintentionally changed the type.Thank you, I have fixed it.