Removed CSolver::Convective_Residual #1222
Conversation
| case FINITE_ELEMENT: | ||
| solver_container[MainSolver]->Convective_Residual(geometry, solver_container, numerics[CONV_TERM], config, iMesh, iRKStep); | ||
| SU2_MPI::Error("Computation of FEM convective flux in non-FEM integration attempted.", CURRENT_FUNCTION); | ||
| break; |
There was a problem hiding this comment.
Thank you for going after dead code 👍
It should also be fairly safe to not cover the FINITE_ELEMENT case or just handle default, I doubt the execution will reach here if the wrong integration class is instantiated (multiple things would have to go wrong).
But looks good to me.
There was a problem hiding this comment.
By the way, there is dead code even in SU2_CFD.cpp, the else branch is unreachable: If it were reached, then turbo and harmonic_balance would both be false. As (multizone && !turbo) has to be false, multizone must be false as well. But then ((!multizone && !harmonic_balance && !turbo) || (turbo && disc_adj)) is true ;)
There was a problem hiding this comment.
I didn't touch this until now because I don't understand the principle behind these nested if-statement. Do you think the else branch in SU2_CFD.cpp should be removed?
There was a problem hiding this comment.
I'm not even going to try unfolding the logic, take it out or throw an error, if the tests pass it's fine by me.
There was a problem hiding this comment.
We can even make the constructors of CFluidDriver protected since it is not meant for standalone use anymore (it exists as the base for the legacy drivers).
There was a problem hiding this comment.
done in commit 7825b3b
There was a problem hiding this comment.
It should also be fairly safe to not cover the FINITE_ELEMENT case or just handle
default, I doubt the execution will reach here if the wrong integration class is instantiated (multiple things would have to go wrong).
I agree, commit 42fab4f
and the corresponding clause removed from SU2_CFD.cpp. If it were reached, then turbo and harmonic_balance would both be false. As (multizone && !turbo) has to be false, multizone must be false as well. But then ((!multizone && !harmonic_balance && !turbo) || (turbo && disc_adj)) is true.
There was a problem hiding this comment.
Thanks @maxaehle for removing unused code 👍 Always really valuable to have those cleanups 🧹
|
When the tests have run, I'll ignore this annoying CodeFactor test and merge, right? |
👍 Yep, I guess this |
Proposed Changes
Remove
CSolver::Convective_Residual, as it does nothing and no derived class overrides it. There is one call inCIntegration.cpp, which I would like to replace by an error message.Note that in the present version, usage of
numerics[CONV_TERM]would not be thread-safe, it should benumerics[CONV_TERM + omp_get_thread_num()*MAX_TERMS](see also #1214).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.