Fusion::lookupValue() #652
Conversation
| val_deque_.push_back(val); | ||
| return getValName(*(val->getValType())); | ||
| const auto vtype = *val->getValType(); | ||
| const auto name = getValName(vtype); |
There was a problem hiding this comment.
A quick question just for clarification. When copying from Fusion 1 to Fusion 2, does the copying of the Vals need to proceed in the same order they are registered in Fusion 1 so that the name lookup will work?
There was a problem hiding this comment.
Good question: when cloning a fusion, the names will be copied directly w/o going through this registration path.
|
Why is it cleaner to have a look up map than it is to track which nodes were created from which? I'm just fundamentally missing why this is a better approach. It seems like a stricter requirement on the fusion copy api than what we're relying on today. |
I tried to answer this in #643. Or more precisely, I tried to explain why the cloning mapping is an internal artifact not intended for external use (and it's not guaranteed to keep working the way it does - future changes to the cloning logic should not impact anything else). Even more importantly, the changes to the Fusion interface needed in order to expose the mapping are deeply problematic. There's also separation of concerns: name to node lookup is a generic and independent operation which can be used for other use cases (ex. binding values for expression evaluation) and it doesn't have to be tied to the place where we do the copy. |
This PR introduced
Fusion::lookupValue(), which looks up a Val node for a specified (vtype, name)It should allow a clean solution for #643