Thy rework ir #3
Conversation
implement real depth first remove external stack in post-order visit abstract Visit trait with auto implem for VisitContext implems
implement Enf, Call, Fold, For, If, Vector, Matrix, Boundary, Parameter
added Eq support to operation few bugfixes
ir/src/ir2/graph/nodes.rs
Outdated
| impl Child for Link<NodeTypes> { | ||
| fn get_parent(&self) -> BackLink<NodeTypes> { | ||
| match self.borrow().deref() { | ||
| NodeTypes::Graph(_) => panic!("get_parent called on graph"), |
There was a problem hiding this comment.
I assume the goal of this NodeTypes is to represent nodes that don't have a parent.
IMO it's clearer to have a NodeTypes::Root that only reference one "Node", and then a Graph is basically a Vec<Root> (or a Graph { boundary_roots: Vec<Root>, integrity_roots: Vec<Root> }).
Would that be possible? Or does this require too much change / is not possible because of some edge case?
The main reason not to do it I think is if this duplicates code to handle adding roots to a graph or similar
There was a problem hiding this comment.
It is possible, but that currently means duplicating the code almost verbatim for the graph builder pattern (modulo what's generic and what's behind the enum variant)
I was thinking of splitting NodeType(s) into multiple categories, but havent yet found a way to make that generic enough to avoid big code duplication. If I find a way to, I'll make it a separate one with just the graph. For now, this has been the easiest to implement/extend upon.
ir/src/ir2/graph/nodes.rs
Outdated
| NodeTypes::I32(ref mut leaf) => leaf.swap_parent(parent), | ||
| NodeTypes::Add(ref mut add) => add.swap_parent(parent), | ||
| NodeTypes::Function(ref mut function) => function.swap_parent(parent), | ||
| _ => panic!("set_parent called on non-node"), |
There was a problem hiding this comment.
maybe "... on a NodeType that does not implement Child" is clearer?
ir/src/ir2/graph/nodes.rs
Outdated
| self.node.get_children().borrow().deref()[1].clone() | ||
| } | ||
| pub fn body(&self) -> Link<NodeTypes> { | ||
| self.node.get_children().borrow().deref()[2].clone() |
There was a problem hiding this comment.
Keep in mind that index access can panic.
I don't think adding error handling is necessary, but maybe we can ensure that Function can only be created through Function::new() and without direct Object instantiation. Here, the field node: Node is private so it's fine, but on Add below it's not for example.
So nothing to change in this case, but just commenting to share :)
ir/src/ir2/graph/nodes.rs
Outdated
| } | ||
| } | ||
| if not_a_node { | ||
| panic!("add_child called on non-node: {:?}", self); |
There was a problem hiding this comment.
weird naming there, I think having an helper function is_node / can_have_children on NodeTypes is clearer. Then, once you check, you can have "unreachable".
and change the message to "on a NodeTypes that does not impl Parent" maybe
There was a problem hiding this comment.
Good call, thats probably a way to customize the node's behaviour in traits with default implementations too, via such helpers.
ir/src/ir2/graph/nodes.rs
Outdated
| use std::ops::{Deref, DerefMut}; | ||
|
|
||
| #[derive(Clone, Eq, PartialEq)] | ||
| pub enum NodeTypes { |
There was a problem hiding this comment.
I think the convention is to use singular NodeType to have, when you reference variants: NodeType::Function(_)
ir/src/ir2/graph/nodes.rs
Outdated
| pub enum NodeTypes { | ||
| Graph(Graph), | ||
| Node(Node), | ||
| I32(Leaf<i32>), |
There was a problem hiding this comment.
Would this be Value(Leaf) basically?
And you'd have something like Variable(Leaf) along side it as the two Leaf types?
There was a problem hiding this comment.
Yes pretty much, the Leaf is just used as a special case where it has no children. It's meant as a way to represent raw values, types, etc. Elements that cannot have sub-nodes essentially. I found it easier to think of it as a link to an arbitrary struct that should not link to other nodes (other than their parent).
Variable probably can be one too, but I need to think about edgecases more before committing to that answer. As a first guess, I agree.
The I32 naming is a relique from my testing, I'll rename to Value.
rename NodeTypes to NodeType split NodeType into LeafNode, RootNode, MiddleNode removed debugging
new_i32 replaced by generic new_value LeafNode::I32 renamed to Value implemented Felt Leaf node improved conversions removed debugging
removed MiddleNode::Node renamed new_body -> new_scope implement Default
Soulthym
force-pushed
the
thy-rework-ir
branch
from
bde9dba
to
750ecf6
Compare
New graph structure for the IR
One bug is left: when using the builder pattern, nodes get duplicated into multiple parents if a call is delegated to a sub-node. I suspect it's due to the automatic unlinking from the old parent (it also removes it from the old parent's children) getting triggered while the parent is still unset.