[GeoMechanicsApplication] Move all non-template code to .cpp files and make sure any .h file does not contain any implementations (retention) #14082
Conversation
rfaasse
left a comment
rfaasse
left a comment
There was a problem hiding this comment.
Thanks for making the retention laws consistent! I only have two minor questions 👍
| KRATOS_CLASS_POINTER_DEFINITION(RetentionLaw); | ||
|
|
||
| class Parameters | ||
| class KRATOS_API(GEO_MECHANICS_APPLICATION) Parameters |
There was a problem hiding this comment.
I haven't seen this used on internal classes (defined within another class). Is it necessary to add the KRATOS_API macro here?
There was a problem hiding this comment.
Yes, it is necessary because I moved the definitions to cpp file.
There was a problem hiding this comment.
Okay, but then it's also necessary for the internal class? I know it's necessary for classes in general, but this parameters class is defined within the retention_law class
There was a problem hiding this comment.
It is not linkable without KRATOS_API(GEO_MECHANICS_APPLICATION) because the functions have been moved to cpp file.
There was a problem hiding this comment.
Ahh apologies, I had missed the functions of this internal object had indeed also moved, thanks for the explanation!
There was a problem hiding this comment.
I think it has to do with the fact that this inner class is being referenced from the unit tests. Since the unit tests are in a separate executable, when building the unit tests the class and all of its members must be accessible. Previously, since the entire class and its definitions were in the header file, this requirement was met (resulting in duplicated machine code: one as part of the GeoMechanicsCore library and the other one as part of the unit test executable). After all, we include that header in the unit tests. Now that the definitions have been moved to the implementation file, that mechanism no longer worked. And also linking to the GeoMechanicsCore library didn't work, since the entire class wasn't exposed. Now that this class is exposed (by using KRATOS_API(GEO_MECHANICS_APPLICATION)), linking will work (and we have a single binary representation of this inner class, which is nice).
applications/GeoMechanicsApplication/geo_mechanics_application.h
Outdated
Show resolved
Hide resolved
rfaasse
left a comment
rfaasse
left a comment
There was a problem hiding this comment.
Good to go! I have a question left, but it's more for curiosity and isn't blocking
📝 Description
A brief description of the PR.
retention_law.htoretention_law.cpp. only inline functions are kept in the header.retention_law_factory.cppto placeClone()definition there.