Skip to content

fix bug in recreate_sim_state - #437

Open
rheiland wants to merge 1 commit into
MathCancer:developmentfrom
rheiland:fix_resume
Open

rheiland wants to merge 1 commit into
MathCancer:developmentfrom
rheiland:fix_resume

Conversation

@rheiland

Copy link
Copy Markdown
Collaborator
  • fix bug when called with unique combination of create_cells and debug_print flags

@drbergman drbergman left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

If we're not creating cells (create_cells = false) should we skip all these checks and (possible) debug prints? As far as I can tell, if create_cells = false, there are no side effects among all these statements other than the debug prints.

Comment on lines +2181 to +2191
int idx_var = pCell->custom_data.find_vector_variable_index( pair.first );
// if it exists, overwrite
if( idx_var > -1 )
{
pCell->custom_data.vector_variables[idx_var].value[jj] = pair.second;
}
else
{
std::cout << __FUNCTION__ << " Error: got an invalid custom data vector name: " << pair.first << " . Exiting! " << std::endl;
std::exit(-1);
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

could lift the checks for this out of the for block over the vector entries.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

All I know is that my test for wanting to see debug prints, but not create cells, resulted in a segfault and these "fixes" avoided that. I'm OK with your logic so long as there are tests included to confirm it. I only discovered the segfault when I was wrapping the "resume" functionality for the Python API, which now works.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants