Support WeakReferences in NodeConnectingVisitor #1057
Conversation
|
I think in CloningVisitor we don't need a weak-ref because we clone the elements, right? |
| if ($this->weakReferences) { | ||
| $parent = \WeakReference::create($parent); | ||
| } | ||
| $node->setAttribute('parent', $parent); |
There was a problem hiding this comment.
One thing I'm wondering about here is whether we should reuse the same attribute names, or use a different one like 'weak_parent', to avoid having two possible types for these attributes.
There was a problem hiding this comment.
We could use separate attribute names, but then I would go with a separate WeakNodeConnectingVisitor class and leave NodeConnectingVisitor untouched ..?
There was a problem hiding this comment.
Given that the implementation themselves are pretty trivial, I'd be fine with adding separate classes as well.
There was a problem hiding this comment.
I am fine with the PR as is, in case you are also ok with it
|
Friendly ping |
There was a problem hiding this comment.
Can you please also update https://github.com/nikic/PHP-Parser/blob/master/doc/component/FAQ.markdown to mention the new option (and why it is recommended to use it)?
There was a problem hiding this comment.
Can you please update the class comment for the new option?
| if ($this->previous !== null) { | ||
| if ( | ||
| $this->weakReferences | ||
| && $this->previous->getAttribute('weak_parent') === $node->getAttribute('weak_parent') |
There was a problem hiding this comment.
I'd split these conditions. If weakReferences we don't want to fall through to the condition below.
|
addressed your feedback, thank you. |
closes #1055
we observed perf improvements on a similar visitor in staabm/phpstan-dba#667 (comment)