-
Notifications
You must be signed in to change notification settings - Fork 358
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
Fixed a bug in the SecondaryEvent delivery #963
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks fine in principle, but see my inline comment.
nestkernel/event.h
Outdated
for ( typename std::vector< DataType >::iterator i = coeffarray_as_d_begin_; | ||
i != coeffarray_as_d_end_; | ||
i++ ) | ||
{ | ||
write_to_comm_buffer( *i, pos ); | ||
elem = *i; | ||
write_to_comm_buffer( elem, pos ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would make this even more compact as
const DataType elem = *i;
And one should probably be able to do it as a one-liner:
write_to_comm_buffer( static_cast< DataType >(*i), pos );
I would prefer that, keeping things as tight as possible.
I would also strongly suggest adding a comment here why this extra effort is necessary.
@heplesser You are right, a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me.
This PR fixes #955. The origin of the error is explained in the corresponding issue.
All SecondaryEvents which exist at the moment use
double
as data type. In this cases the sizes in question are the same (this is actually the case for most data types that I checked), which is why this error remained undetected until now (and cannot be tested with the current master).I have created a branch (based on 5g) where I created an additional SecondaryEvent with
bool
data (see https://github.com/janhahne/nest-simulator/tree/5g_bool_sec_event). After merging the fix everything works fine there now.