[GeoMechanicsApplication] Move all non-template code to .cpp files and make sure any .h file does not contain any implementations (processes) #14075
Conversation
rfaasse
left a comment
rfaasse
left a comment
There was a problem hiding this comment.
Thanks for the efforts of splitting classes into header + source, improving the 'include health' in a lot of these classes and also making the changes in a way that makes diffing as easy as possible (i.e. you kept the same order in the functions, that's much appreciated 😄 ).
I didn't do an in-depth review of all code, but mainly checked it stayed the same and all implementations were moved out of the headers. I only have a few minor comments about forward declares, includes and some left-over implementations in .h files.
| namespace Kratos | ||
| { | ||
| class ModelPart; | ||
| class Parameters; |
There was a problem hiding this comment.
Since the Parameters are passed to the constructor by value, a forward declare does not work (I guess the parameters are included implicitly somehow). This same comment applies to other processes
| { | ||
|
|
||
| class Model; | ||
| class Parameters; |
There was a problem hiding this comment.
Here the forward declare does work, because the Parameters are passed as reference 👍
| { | ||
|
|
||
| class ModelPart; | ||
| class Parameters; |
There was a problem hiding this comment.
Here again it shouldn't work, since the parameters are passed by value. I won't mention it for the rest of the processes, but it's something to double-check 👍
| /// output stream function | ||
| inline std::ostream& operator<<(std::ostream& rOStream, const ApplyConstantInterpolateLinePressureProcess& rThis) | ||
| { | ||
| rThis.PrintInfo(rOStream); | ||
| rOStream << std::endl; | ||
| rThis.PrintData(rOStream); | ||
|
|
||
| return rOStream; | ||
| } |
There was a problem hiding this comment.
Could this also be moved to the cpp?
There was a problem hiding this comment.
I think it is possible but then it could not be inline, right? As far as I remember a compiler makes a decision and it can ignore inline statements.
|
|
||
| namespace Kratos | ||
| { | ||
| using namespace std::string_literals; |
There was a problem hiding this comment.
std::string_literals is defined in the header. For completeness I think it's a good idea to include it here (I guess now it gets in somewhere implicitly). This probably also holds for other processes
|
|
||
| #include "processes/process.h" | ||
|
|
||
| #include <memory> |
There was a problem hiding this comment.
We use std::unique_ptr in this file, so we should keep this include
| inline std::istream& operator>>(std::istream& rIStream, ApplyWriteScalarProcess& rThis); | ||
|
|
||
| /// output stream function | ||
| inline std::ostream& operator<<(std::ostream& rOStream, const ApplyWriteScalarProcess& rThis) |
There was a problem hiding this comment.
Could this implementation also be moved to the cpp?
| inline std::ostream& operator<<(std::ostream& rOStream, const ApplyPhreaticMultiLinePressureTableProcess& rThis) | ||
| { | ||
| rThis.PrintInfo(rOStream); | ||
| rOStream << std::endl; | ||
| rThis.PrintData(rOStream); | ||
|
|
||
| return rOStream; | ||
| } |
There was a problem hiding this comment.
Could this one also be moved to the cpp?
| inline std::ostream& operator<<(std::ostream& rOStream, const DeactivateConditionsOnInactiveElements& rThis) | ||
| { | ||
| rThis.PrintInfo(rOStream); | ||
| rOStream << std::endl; | ||
| rThis.PrintData(rOStream); | ||
|
|
||
| return rOStream; | ||
| } |
There was a problem hiding this comment.
Could this implementation also be moved to the cpp?
📝 Description
A brief description of the PR.