Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix #284, "fixed_step_algorithm crashes when a variable is reconnected" #285

Conversation

kyllingstad
Copy link
Member

The problem was that fixed_step_algorithm::impl::disconnect_variable() didn't check whether it pointed to an actual element before trying to erase it.

This PR also fixes an unrelated hidden bug in the same function: The conns variable was a copy of the connection list, and not a reference to it as was intended. This would have rendered the entire function a no-op.

The problem was that fixed_step_algorithm::impl::disconnect_variable
didn't check whether `it` pointed to an actual element before trying to
erase it.

This also fixes an unrelated hidden bug in the same function:
The `conns` variable was a *copy* of the connection list, and not a
reference to it as was intended.  This would have rendered the entire
function a no-op.
@kyllingstad kyllingstad added the bug Something isn't working label Jun 26, 2019
@kyllingstad kyllingstad requested review from mrindar and eidekrist June 26, 2019 06:04
@kyllingstad kyllingstad self-assigned this Jun 26, 2019
@mrindar
Copy link
Contributor

mrindar commented Jun 26, 2019

Why are we trying to disconnect or erase a connection that doesn't exist?

EDIT:
Ah. So the function loops through all the simulators, looking for the one that has the variable in question as an outgoing connection, and erases it. Previously it crashed, because for it to work the variable had to, by chance, be in the first simulator in the simulators_ list?

Copy link
Member

@eidekrist eidekrist left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great! It would however be nice to see this tested - just something as simple as adding the same connection twice.

@kyllingstad
Copy link
Member Author

@mrindar:

Ah. So the function loops through all the simulators, looking for the one that has the variable in question as an outgoing connection, and erases it.

Almost. It looks for the one that has an output variable which is connected to the input variable in question.

Previously it crashed, because for it to work the variable had to, by chance, be in the first simulator in the simulators_ list?

The loop body would crash for all simulators that did not have the connection. Maybe we just haven't tested this functionality yet, or maybe we've only tested it in cases where simulators_ contained only the one simulator.

@kyllingstad
Copy link
Member Author

@eidekrist:

Great! It would however be nice to see this tested - just something as simple as adding the same connection twice.

And here I was hoping to get away without it. :D But I agree and will fix.

@kyllingstad
Copy link
Member Author

Test added now.

@kyllingstad kyllingstad requested a review from eidekrist July 22, 2019 11:04
@ljamt ljamt self-requested a review July 23, 2019 07:35
Copy link
Member

@ljamt ljamt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good 👍
As the test requested by @eidekrist is included now we can merge.

@kyllingstad kyllingstad merged commit dc1851e into master Jul 23, 2019
@kyllingstad kyllingstad deleted the bugfix/284-fixed_step_algorithm-crashes-when-a-variable-is-reconnected branch July 23, 2019 07:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants