Clean up AIGER graph modules#2
Conversation
sampsyo
left a comment
sampsyo
left a comment
There was a problem hiding this comment.
Looks great from here! I have two random comments and one suggestion. When you're happy, I suggest going ahead and merging this!
| /// see `impl TryFrom<NodeId> for usize` the specific logic of this, and | ||
| /// `impl From<usize> for NodeId` for converting the other way (i.e. graph index to NodeID) | ||
| fn index(self) -> usize { | ||
| usize::try_from(self).expect("NodeId does not correspond to a graph index") |
There was a problem hiding this comment.
FWIW, I think this can be written slightly more simply as:
self.try_into().expect(...)
Rust should infer the return type for you.
| /// graph[0] -> NodeId(2) | ||
| /// graph[1] -> NodeId(4) | ||
| /// graph[2] -> NodeId(6) | ||
| impl From<usize> for NodeId { |
There was a problem hiding this comment.
There is nothing specific to do about this, but just noting here that it is a teensy bit surprisingly that the two conversion directions have different failure behavior. (ID -> index can yield an error; index -> ID can only panic.) I suppose that's appropriate given the failure conditions, but just noting that it can look odd!
| self.graph.node(id) | ||
| } | ||
|
|
||
| pub fn add_input(&mut self) -> NodeId { |
There was a problem hiding this comment.
Another non-actionable thing, but possibly something to keep in mind: when I proposed the AigBuilder refactor, I was also wondering whether these methods (add_*) should go in the builder or the graph itself.
The argument for going in the builder (i.e., what you have now) is, of course, that these methods are used to build up the graph.
The argument for going in the graph itself is that they are also useful for "editing" an existing graph (i.e., adding a small number of new nodes and such, but not "bulk processing" a conversion like we do during parsing). So clients like that might want to have these methods available even though they don't need the and_hash deduplication trick.
Anyway, maybe we should just leave it as it is for now, and keep this in mind as we write more code that manipulates graphs?
2 participants
No description provided.