[Exp] Add GetMaxDepth #11970
[Exp] Add GetMaxDepth #11970
Conversation
| return (*std::max_element( | ||
| mSourceExpressions.begin(), | ||
| mSourceExpressions.end(), | ||
| [](const auto& rV1, const auto& rV2) { | ||
| return rV1->GetMaxDepth() < rV2->GetMaxDepth(); | ||
| }))->GetMaxDepth() + 1; | ||
| } |
There was a problem hiding this comment.
that's a lot of repeated depth calculations. It might be worth to consider adding a depth member variable that gets computed at construction time (provided that child Expressions are immutable), instead of always recomputing every depth for each child over and over again.
There was a problem hiding this comment.
I agree, but I don't see it getting called several times. [At the moment, it will only be called when the future Collapse is called, but that happens when we Collapse at the end. There is no point in Collapsing every time]. I would say, we can move it to a member variable if this gets out of hand.
In any case, I simpliifed the recursions, so there are no redundant recursive calculations.
There was a problem hiding this comment.
I added a warning as well for future reference.
|
@matekelemen I have addressed your comments. Could you have a look? |
📝 Description
This PR adds
Expression::GetMaxDepthto compute the maximum depth of the lazy expression tree structure.🆕 Changelog