Conversation
|
|
|
Since I am already at it, I am gonna add factory classes for numerics (we can use them even if we move the allocation to the solvers) and integration. |
economon
left a comment
There was a problem hiding this comment.
LGTM. The factory pattern is a good one that we should have been using for a long time. I am for adding this for the integration / numerics as well.
|
|
||
| for (unsigned short iSol = 0; iSol < MAX_SOLS; iSol++){ | ||
| if (solver_container[iZone][INST_0][MESH_0][iSol] != NULL){ | ||
| if (solver_container[iZone][INST_0][MESH_0][iSol] != nullptr){ |
| #include "../../include/solvers/CIncEulerSolver.hpp" | ||
| #include "../../include/solvers/CNSSolver.hpp" | ||
| #include "../../include/solvers/CIncNSSolver.hpp" | ||
| #include "../../include/solvers/CTurbSASolver.hpp" |
There was a problem hiding this comment.
Getting all of the headers out of CDriver is great 👍
SU2_CFD/src/drivers/CDriver.cpp
Outdated
| void CDriver::Solver_Preprocessing(CConfig* config, CGeometry** geometry, CSolver ***&solver) { | ||
|
|
||
| unsigned short iSol; | ||
There was a problem hiding this comment.
well well, what do we have here 🕵 (Detective Whitespace on the hunt)
SU2_CFD/src/drivers/CDriver.cpp
Outdated
|
|
||
| delete [] solver[val_iInst]; | ||
|
|
||
There was a problem hiding this comment.
🕵 This is one of the easier cases for Detective Whitespace
| * \param kindSubSolver - The kind of sub solver | ||
| * \return - Pointer to the allocated integration instance | ||
| */ | ||
| static CIntegration* createIntegration(INTEGRATION_TYPE integrationType); |
There was a problem hiding this comment.
integrationType instead of kindSubSolver (although integrationType directly depends on kindSubSolver as far as I understand)
There was a problem hiding this comment.
Could we remove the functions Solver/Integration_Preprocessing (and all the *_Preprocessing routines that follow) and pull the few lines up to the CDriver constructor as it is only called from there.
This function has 4 lines of code and only hands work over to the factory 🏭 (why didn't I think earlier of using the factory emoji 🤦♂ )
There was a problem hiding this comment.
I will leave it like that for the moment. We can consider a clean-up once we have factory methods for the rest of the containers (especially numerics).
| * \param[in] kindSolver - The kind of main solver | ||
| * \return - Pointer to the allocated Output | ||
| */ | ||
| static COutput* createOutput(ENUM_MAIN_SOLVER kindSolver, CConfig *config, int nDim); |
| /*! | ||
| * \brief Private constructor to avoid creating instances of this class | ||
| */ | ||
| COutputFactory(); |
There was a problem hiding this comment.
Thanks for the const stuff, everything looks very nice.
I think it is a good idea to have a separate numerics PR.
Final note, the preferred C++11 way to achieve this is to delete the default constructor instead of making it private. COutputFactory() = delete;
There was a problem hiding this comment.
Perfect, thanks! If that's the preferred way then we should do it 👍
There was a problem hiding this comment.
At least according to Scott Meyers xD
|
As all comments are addressed, I am merging this in. Thanks @talbring for the effort 💐 |
Proposed Changes
In order to structure the construction of the solvers a little bit and to remove the dependency of the driver on the solver, I created a static solver factory class. There are routines to create individual instances as well as the whole container. It should be much easier now to see what position in the containers are actually allocated for the different solvers.
The
createSolverroutine is a generic one that simply callsnew. The others have some additional checks or calls for the construction. The idea here was to remove anyifstatements in thecreateSolverContainerroutine to ease readability.If everybody agrees, we could use a similar structure for other classes as well. Let me know what you think.
Note from TobiKattmann: This PR depends on #861
Related Work
Resolve any issues (bug fix or feature request), note any related PRs, or mention interactions with the work of others, if any.
PR Checklist
Put an X by all that apply. You can fill this out after submitting the PR. If you have any questions, don't hesitate to ask! We want to help. These are a guide for you to know what the reviewers will be looking for in your contribution.