Improve tree/node copying #57
Conversation
grammarinator/runtime/tree.py
Outdated
| @@ -19,6 +19,11 @@ def __init__(self, root): | |||
| self.root = root | |||
| self.node_dict = None | |||
|
|
|||
| def __deepcopy__(self, memo): | |||
| tree = Tree(deepcopy(self.root)) | |||
There was a problem hiding this comment.
shouldn't memo be passed on to deepcopy?
There was a problem hiding this comment.
I'm not sure. deepcopy(self.root) creates a deepcopy of a tree hierarchy rooted from self.root which is different from the Tree object. It calls the default deepcopy implementation which initiates memo to an empty dict if it's not defined, which is perfect for us. Besides, these tree hierarchies must not contain recursive loops anyhow (except if they were manually crafted so, but that's a bad design and it will fail a lot of other ways probably).
There was a problem hiding this comment.
Alright, as the current implementation stands, memo may not be of big use. But what can go wrong if we do pass on memo? Wouldn't it make the implementation more future-proof? (Btw, loops are not the only construct where memo can help. DAGs may also benefit from memo, if I get it right. But certainly, we cannot have DAGs either at the moment.)
There was a problem hiding this comment.
We can do that, it won't cause any harm indeed (since it will execute exactly the same code path as now). But once again, the current default implementation - which initiates the unset memo parameter to dict - also handles a possible recursion (or even DAGs).
The patch removes the superfluous copy/deepcopy methods from tree nodes, since they only called the default implementation. On the other hand, it defines the way of deepcopying a whole tree, which means deepcopying the root node and annotate the new tree.
The patch removes the superfluous copy/deepcopy methods from tree nodes, since they only called the default implementation. On the other hand, it defines the way of deepcopying a whole tree, which means deepcopying the root node and annotate the new tree.