basic version of eval function#3
Conversation
sampsyo
left a comment
sampsyo
left a comment
There was a problem hiding this comment.
Awesome; seems like a great start. My recommendation for the next micro-step here would be to set up some tests so you can "lock in" the current behavior of this evaluator before adding features/performance/etc.
| use super::{AigGraph, NodeId}; | ||
|
|
||
| impl AigGraph { | ||
| pub fn eval(&self, id: NodeId, input_values: &HashMap<NodeId, usize>) -> usize { |
There was a problem hiding this comment.
Just to set up for future changes, I recommend using a couple of type aliases here:
type Value = usize;
type Env = HashMap<NodeId, Value>;| match id { | ||
| id if id.is_false() => 0, | ||
| id if id.is_true() => 1, | ||
| id if id.is_inverted() => 1 - (self.eval(id.regular(), input_values)), | ||
| id => { |
There was a problem hiding this comment.
While I understand the temptation to use match here, I think in this case, I'm not sure it successfully makes things much more readable than an if ... else if ... cascade (because we're not actually matching any patterns). I suppose either way can work, but you might find that the latter parts of this function (where you check for properties of the node itself) would be a little easier (and less indented) with a normal cascade and early returns.
There was a problem hiding this comment.
Will do! I was pretty torn between using match and if else, and I do agree that if else would prob be better here, the match wasn't as "clean" as I thought it would be
… use alias for input map and values
sampsyo
left a comment
sampsyo
left a comment
There was a problem hiding this comment.
Nice. I think it did end up being a little more readable with the else if cascade rather than match.
I say go ahead and merge this! Maybe testing is the next step, and that could go in a separate PR?
2 participants
No description provided.