correcting solver builder constructor #616
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #616 +/- ##
=======================================
Coverage 93.51% 93.51%
=======================================
Files 49 49
Lines 3502 3502
=======================================
Hits 3275 3275
Misses 227 227 ☔ View full report in Codecov by Sentry. |
| std::swap(this->solver_, other.solver_); | ||
| std::swap(this->processes_, other.processes_); |
There was a problem hiding this comment.
@K20shores @mattldawson I thought doing swap here would make sure when other is destroyed by the move constructor, it did not free the space now owned by this. Kyle's change basically means this is not necessary?
There was a problem hiding this comment.
This Solver(&&) move constructor caused an error in musica, and I'm working on it. Also the move assignment has a bug in it. We might have to add test for this.
Solver& operator=(Solver&& other)
{
std::swap(this->solver_, other.solver_);
number_of_grid_cells_ = other.number_of_grid_cells_;
number_of_species_ = other.number_of_species_;
number_of_reactions_ = other.number_of_reactions_;
state_parameters_ = other.state_parameters_;
std::swap(this->solver_, other.processes_); <---- this should get corrected
return *this;
}There was a problem hiding this comment.
Ah, thanks for catching this typo. I wonder whether we really need a move assignment in a actual case. If not, would it be easier if we just delete the move assignment operator?
There was a problem hiding this comment.
no problem! yeah I don't see the need to have move assignment.
Correct the constructor of the solver. Apparently this was preventing part of musica from building. I don't understand it