Conversation
cmyr
left a comment
cmyr
left a comment
There was a problem hiding this comment.
I see the benefit. I'm a little hesitant about needs_merge; I think this method is getting to the point where we really want to start worrying about its complexity and the difficulty of reasoning about behaviour as we make changes in the future, and I think that it might be worth optimizing for reduced complexity at the cost of small runtime costs, and in the cases where merging would be skipped I expect skipping will be at best a verrrry small win?
druid/src/core.rs
Outdated
| // If we need to replace either the event or its data. | ||
| let mut modified_event = None; | ||
| // Tracking whether we need to merge our state up to the parent. | ||
| let mut needs_merge = false; |
There was a problem hiding this comment.
is this an optimization (my assumption), or are there times when merging would be incorrect?
if an optimization: my gut feeling is that merging is only a trivial amount of work in the general case, such that this optimization is unlikely to be meaningful even over very large graphs, and I'm not sure it's something it is worth adding more logic to handle, at least without being guided by a concrete performance concern?
|
I don' think the |
2 participants
This PR is just the refactoring part of #986 so that we can get the refactoring in first and make the command targeting work more clear.
This new approach is more like
WidgetPod::lifecycle. It doesn't copy/clone as much and is fewer lines of code allowing for easier understanding.