Graph Changes #37535
Graph Changes #37535
Conversation
Use where clasues and only where clauses for bounds in the iterators for Graph. The rest of the code uses bounds on the generic declarations for Debug, and we may want to change those to for consistency. I did not do that here because I don't know whether or not that's a good idea. But for the iterators, they were inconsistent causing confusion, at least for me.
Also used those general iterators in other methods.
|
r? @eddyb (rust_highfive has picked a reviewer for you, use r? to override) |
| fn is_node_cyclic_b() { | ||
| let graph = create_graph_with_cycle(); | ||
| assert!(graph.is_node_cyclic(NodeIndex(1))); | ||
| } |
There was a problem hiding this comment.
Missing newline at the end of the file.
|
cc @nikomatsakis Are we fine with such additions? |
|
here it comes: Using the convention of naming the iterator struct exactly like the method that creates it would be good. I see that the existing outgoing_edges doesn't do that, but it's not too late for new additions to do so. |
| @@ -20,10 +20,13 @@ fn create_graph() -> TestGraph { | |||
|
|
|||
| // Create a simple graph | |||
| // | |||
| // F | |||
| // | | |||
| // V | |||
| // A -+> B --> C | |||
There was a problem hiding this comment.
This should probably become
// A --> B --> C
(i.e. '+' should become '-')
There was a problem hiding this comment.
Oh whoops. I noticed that the comment was wrong in general about the placement of f, so moved it to be correct, but forgot to update the + there. Also fixed in the other comment where I just removed F.
|
@bluss So based on that, what names are you thinking? Because I think you're thinking |
|
@Havvy Well I didn't propose anything about that, only that the struct should be named like the method or vice versa. So for example Spontaneously, maybe |
|
I like those method names better, and went with them, and also made the Iterator structs match the method names. |
|
I have no objection to extending the graph API, but I would say we have no intention of supporting it. It'd probably be better for clippy to use petgraph or something on |
|
@nikomatsakis This is for using |
|
@eddyb it'd probably be better for clippy to recreate the graph in petgraph then =) |
|
but yeah I'm fine w/ landing these changes, particularly since |
|
Is |
alexcrichton
added
the
T-compiler
|
@Havvy thoughts about extracting this data structure to crates.io, perhaps build on petgraph? If what @nikomatsakis says is correct then this is soon to be deleted anyway. |
|
There's a similar lint already in the compiler (infinitely recursive functions) as to what I am attempting to do (though I've put in on hold for a couple days because, yay trying to figure out what's staying and what's changing), so I can just use what that lint uses. I don't need So my question is, is the Graph structure used outside of cfg. If so, these commits are still probably useful (except probably the last one), but if not, there's no point merging this and it should be closed. So, what is the future of Graph? |
|
To be clear, I have no objection to landing this PR for the time being. I just want to be clear that there are no promises of backwards compatibility etc. |
|
@bors r+ |
|
📌 Commit 9ddbb91 has been approved by |
Graph Changes General cleanup and adding a few methods that I want to use in Clippy. Need somebody to bikeshed names.
Graph Changes General cleanup and adding a few methods that I want to use in Clippy. Need somebody to bikeshed names.
Rollup of 30 pull requests - Successful merges: #37190, #37368, #37481, #37503, #37527, #37535, #37551, #37584, #37600, #37613, #37615, #37659, #37662, #37669, #37682, #37688, #37690, #37692, #37693, #37694, #37695, #37696, #37698, #37699, #37705, #37708, #37709, #37716, #37724, #37727 - Failed merges: #37640, #37689, #37717
General cleanup and adding a few methods that I want to use in Clippy.
Need somebody to bikeshed names.