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

Correctly initialize result vector in music_rate_in_proxy::get_status() #1458

Merged
merged 4 commits into from
Mar 9, 2020

Conversation

hakonsbm
Copy link
Contributor

@hakonsbm hakonsbm commented Mar 9, 2020

Corrects the type of the Buffers_::data_ variable in the MUSIC rate in proxy model.

With the original uninitialised double, passing the variable to the std::vector constructor may lead to an std::bad_alloc error. This can happen when the model tries to copy the vector with

( *d )[ names::data ] = DoubleVectorDatum( new std::vector< double >( B_.data_ ) );

This PR changes the variable to an std::vector.

Fixes #1449.

Copy link
Contributor

@heplesser heplesser left a comment

Choose a reason for hiding this comment

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

See inline.

@@ -167,7 +167,7 @@ class music_rate_in_proxy : public DeviceNode

struct Buffers_
{
double data_;
std::vector< double > data_; //!< The buffer for incoming data
Copy link
Contributor

Choose a reason for hiding this comment

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

@hakonsbm Based on your previous analysis this update makes sense, but I wonder if a smaller change, namely changing

( *d )[ names::data ] = DoubleVectorDatum( new std::vector< double >( B_.data_ ) );

to

( *d )[ names::data ] = DoubleVectorDatum( new std::vector< double >( 1, B_.data_ ) );

so that we correctly create a single-element vector, would be the safer approach here to just fix this problem?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. But we should really have a test of this model.

Copy link
Contributor

Choose a reason for hiding this comment

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

Indeed, would you create a follow-up issue for a proper review of this (and other music) proxies?

@heplesser heplesser added ZC: Kernel DO NOT USE THIS LABEL I: No breaking change Previously written code will work as before, no one should note anything changing (aside the fix) ZP: PR Created DO NOT USE THIS LABEL S: Critical Needs to be addressed immediately T: Bug Wrong statements in the code or documentation labels Mar 9, 2020
@heplesser heplesser requested a review from jakobj March 9, 2020 14:44
@heplesser heplesser changed the title Correct type of variable in MUSIC rate in proxy model Correctly initialize result vector in music_rate_in_proxy::get_status() Mar 9, 2020
Copy link
Contributor

@jakobj jakobj left a comment

Choose a reason for hiding this comment

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

looks good, thanks! 👍 conditioned on travis

@heplesser heplesser merged commit c16844f into nest:master Mar 9, 2020
@hakonsbm hakonsbm deleted the music_rate_proxy_init_fix branch March 10, 2020 08:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I: No breaking change Previously written code will work as before, no one should note anything changing (aside the fix) S: Critical Needs to be addressed immediately T: Bug Wrong statements in the code or documentation ZC: Kernel DO NOT USE THIS LABEL ZP: PR Created DO NOT USE THIS LABEL
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

GetDefaults occasionally causes bad_alloc if NEST is built with MUSIC
3 participants