add assert to notify incurred UB in case a node was linked during send_down

[menzels]

similar to the notify loop
#212

it is possible to invalidate the iterators when new cursors are created during send_down for example in a reader::map() execution.

this should be fixed by using index based for loop analog to the notify function.

another solution would be to trow an error if that happens if one should not use that pattern.

[arximboldi]

What are you doing in your reader::map() to trigger that? I am curious about what you are trying to achieve.

The callback to reader::map() should ideally be pure, and therefore not cause such things. recompute() should not cause children to change, because it should just derive the child value from its parents values. If this is not the case the whole model kind of falls apart I reckon. The throwing error is therefore a better approach. I imagine checking if children changed would have some performance cost, so perhaps this can be done only in debug mode.

[menzels]

yea i was doing something like this:

LAGER_QT(deckModel) =_store[&AppState::deck].map(std::bind(&createDeckModelPtr, std::ref(_store), _1));

with

inline DeckModelPtr createDeckModelPtr(StoreType& store, Deck deck) {
  DeckModelPtr ptr = new DeckModel(store[&AppState::deck]);
  QQmlEngine::setObjectOwnership(ptr, QQmlEngine::JavaScriptOwnership);
  return ptr;
}

with further testing and thinking i think it makes no sense like this and i changed the approach.
it still took some time to find out why it crashed sometimes and somtimes some updates were not processed anymore.

ill see if i can find a way to notify if this happens during debugging.

[arximboldi]

I see :) If you're using JavaScriptOwnership it's often better to just create those objects lazily on some sort of getter. Otherwise, I would use reader.watch([]{}) to add reactions to model changes that produce side-effects. It sounds like you probably noticed already some of this.

If you find a cheap way to test for this mistake at runtime feel free to update the PR :)

[menzels]

i added an assert to test this mistake, there should be no cost in release build.
i wanted to add a test for this, but catch2 does not support checking asserts or termination sadly.

ill squash and edit the pr title soon.