augment TerminalNode with setParent() #1674
Conversation
…ard compatible as it changes the interface but no one was able to create custom TerminalNodes anyway so I'm adding as it improves internal code quality. addChild now sets the parent rather than create. MUCH better.
| TerminalNodeImpl node = new TerminalNodeImpl(t); | ||
| node.parent = parent; | ||
| return node; | ||
| return new TerminalNodeImpl(t); |
There was a problem hiding this comment.
The parameter parent should go. It's no longer used and produces warnings e.g. in the C++ target.
There was a problem hiding this comment.
the leaf might be a function of the parent though, right?
There was a problem hiding this comment.
Hmm, in what sense? The parent is set by the caller, so why pass it into this method?
There was a problem hiding this comment.
Well, one might want to create a different kind of leaf (i.e., type) if the parent is an expression versus a declaration. For example, an ID token might become VarRef vs VarDef leaf? Just leaving it flexible but I can see removing it if you guys feel strongly.
There was a problem hiding this comment.
Ok, I see your point. Have to manually take care about that parameter then in C++ (not the first time, there are many other places with the same kind of issue).
| ErrorNodeImpl node = new ErrorNodeImpl(t); | ||
| node.parent = parent; | ||
| return node; | ||
| return new ErrorNodeImpl(t); |
| this.children.add(child); | ||
| ((ErrorNodeImpl) child).parent = this; | ||
| if ( child instanceof ErrorNode ) { | ||
| addChild((ErrorNode)child); |
There was a problem hiding this comment.
This is a bit weird code. You are not copying the error node, but actually create references from 2 contexts and only set the new context as parent. However, it's still in the source context, since you don't remove it from there. What is the real intention here? Really move the node or create a duplicate (like the method name suggests).
There was a problem hiding this comment.
The error nodes should be moved to the new context in copyFrom as it's used only for flipping contexts per the comment.
There was a problem hiding this comment.
As I said, a bit weird. You have a copy function which moves something :-) At least I have it then like you want. But the Java code is still buggy. The error node is still in the children list of the source context, since you don't remove it from there.
There was a problem hiding this comment.
The node copied from is candidate for GC as all refs to it disappear; figured it's a waste to alter it when it's dead. Maybe a better comment? :)
There was a problem hiding this comment.
Well, that is implicit knowledge, which is not visible at this point in code and for some targets like C++ it is very important to know what is managing the memory. And if you one day decide to keep the source context around you certainly forgot by that time that it contains nodes which are supposed to be in a different context.
| @@ -139,12 +139,18 @@ public RuleContext addChild(RuleContext ruleInvocation) { | |||
| return addAnyChild(ruleInvocation); | |||
There was a problem hiding this comment.
You don't want to set the parent of the ruleInvocation context like you do for terminal and error nodes?
There was a problem hiding this comment.
no, that is bound up in the parsing functionality; i know it's odd.
|
Also, what is the meaning of a setter for a property which is directly accessbile? The |
|
@mike-lischke TerminalNode is an interface not a class so no field is available. |
An addendum to #1665
Technically, this is not backward compatible as it changes the interface but no one was able to create custom TerminalNodes anyway so I'm adding as it improves internal code quality. addChild now sets the parent rather than create. MUCH better.