[GeoMechanicsApplication] Move all non-template code to .cpp files and make sure any .h file does not contain any implementations (constitutive) #14053

markelov208 · 2025-12-09T14:37:39Z

📝 Description
A brief description of the PR.

Actions have been done for files in custom_constitutive folder

moved GetStrainSize function to cpp file
renamed hpp to h

…-cpp-constitutive

rfaasse

Hi @markelov208, thanks for the effort for all these PRs! I didn't find any issues, so in principle it's ready to go! The only thing I'd like to wait for is a short discussion about the =default for constructors etc: do we want it in the header or the source?

rfaasse · 2025-12-18T15:01:27Z

...tions/GeoMechanicsApplication/custom_constitutive/incremental_linear_elastic_interface_law.h

+    explicit GeoIncrementalLinearElasticInterfaceLaw(std::unique_ptr<ConstitutiveLawDimension> pConstitutiveLawDimension);

-    ~GeoIncrementalLinearElasticInterfaceLaw() override = default;
+    ~GeoIncrementalLinearElasticInterfaceLaw() override;


I'm starting to think we should keep default constructors/destructors/operations etc (defined with =default) in the header. As we saw previous PR in the following discussion https://stackoverflow.com/questions/62580224/should-functions-declared-with-default-only-go-in-the-header-file, this could be indeed the case (I didn't find anything explicitly in the core guidelines).

At least I have no strong preference anymore towards moving it to the cpp and as we've seen, it results in extra SonarCloud smells. @avdg81 Do you want to weigh in here?

Good question. I'll check some textbooks to see whether there are any recommendations that support what you've found on SO.

To be able to move forward with this (and the other PRs), my proposal would be to keep =default in the header when possible (the exception indeed being forward declared variables in unique pointers, which force us to put the destructor in the source file), since I don't see any good reasons not to do it this way (and some reasons to do so).

If we find any strong argument to have default constructors/destructors in the source file of course we can still change it later.

I moved default destructors and constructors to headers. The only weird thing is that forward declaration of ConstitutiveLawDimension stopped working in GeoIncrementalLinearElasticLaw.

To be able to move forward with this (and the other PRs), my proposal would be to keep =default in the header when possible (the exception indeed being forward declared variables in unique pointers, which force us to put the destructor in the source file), since I don't see any good reasons not to do it this way (and some reasons to do so).

If we find any strong argument to have default constructors/destructors in the source file of course we can still change it later.

Indeed, as I mentioned here, that is the exception. So if there is a forward declared unique pointer (in this case of the ConstitutiveDimension), the destructor needs to be in the .cpp for it to compile

avdg81

Hi Gennady,
Thanks a lot for moving member function definitions to the corresponding implementation files. Also thanks for changing a few file extensions from .hpp to .h. Apart from a minor (and non-blocking) suggestion, I feel this PR is good to go.

applications/GeoMechanicsApplication/custom_constitutive/small_strain_umat_2D_interface_law.cpp

…-cpp-constitutive

WPK4FEM

To me this looks like a consistent batch of changes for the custom_constitutive directory. The pipeline is green and the earlier comments by Anne and Richard were taken up and apparently resolved.

rfaasse · 2026-01-06T08:37:50Z

applications/GeoMechanicsApplication/custom_constitutive/incremental_linear_elastic_law.h

-    ~GeoIncrementalLinearElasticLaw() override;
+    GeoIncrementalLinearElasticLaw(GeoIncrementalLinearElasticLaw&& rOther) noexcept = default;
+    GeoIncrementalLinearElasticLaw& operator=(GeoIncrementalLinearElasticLaw&& rOther) noexcept = default;
+    ~GeoIncrementalLinearElasticLaw() override = default;


As discussed in the other thread, the default destructor in case of forward declaration is the exception, that one needs to be in the cpp file, such that we can forward declare ConstitutiveLawDimension and have the include in the cpp file instead of here in the header (and improve compilation times)

I added comments to explain this in other places (see e.g. https://github.com/KratosMultiphysics/Kratos/blob/master/applications/GeoMechanicsApplication/custom_workflows/time_loop_executor.cpp#L24-L26), but here it might've been unclear, apologies.

So if you could move the relevant default destructors to the cpp (so only the ones that are needed to be able to compile forward declared unique pointer member variables), I think this PR is good to go!

Hi Richard, I move the destructor for GeoIncrementalLinearElasticInterfaceLaw. As well I removed a false forward declaration in small_strain_udsm/umat_law.

Richard, please could you take a look on code smells due to my recent changes? Thank you.

Hi Gennady, yes, having the destructor in the cpp file results in a code smell, but in my opinion it's worth it due to being able to forward declare classes. If you and/or @avdg81 disagree, we could discuss or revert.

avdg81

Hi Gennady,
Thanks for processing my review suggestion. I believe we're almost ready to merge this PR. I'm not approving it yet, since one of Richard's suggestions is still open, see this comment.

rfaasse

For me this PR is ready to go, thanks for all the effort that went into this!

renamed hpp into h

65e2434

markelov208 self-assigned this Dec 9, 2025

markelov208 requested a review from a team as a code owner December 9, 2025 14:37

markelov208 linked an issue Dec 9, 2025 that may be closed by this pull request

[GeoMechanicsApplication] Move all non-template code to .cpp files and make sure any .h file does not contain any implementations #14046

Closed

markelov208 added 2 commits December 11, 2025 19:15

Merge branch 'master' into geo/14046-move-non-template-code-from-h-to…

84874fd

…-cpp-constitutive

second part

888d6d0

markelov208 requested review from WPK4FEM, avdg81 and rfaasse December 12, 2025 09:51

markelov208 added 2 commits December 15, 2025 14:27

moved GetStrainMeasure/GetStressMeasure from hpp to cpp

6f8b6dd

forgotten file

0b9fd2d

rfaasse reviewed Dec 18, 2025

View reviewed changes

avdg81 reviewed Dec 19, 2025

View reviewed changes

applications/GeoMechanicsApplication/custom_constitutive/small_strain_umat_2D_interface_law.cpp Outdated Show resolved Hide resolved

markelov208 added 2 commits December 19, 2025 14:16

added using namespace std::string_literals;

9e9da82

Merge branch 'master' into geo/14046-move-non-template-code-from-h-to…

ce0aa39

…-cpp-constitutive

markelov208 requested review from avdg81 and rfaasse December 22, 2025 08:42

markelov208 added 2 commits December 23, 2025 14:38

moved default con/destructors to headers

4e17197

moved default constructor

aa04e05

WPK4FEM previously approved these changes Jan 5, 2026

View reviewed changes

rfaasse reviewed Jan 6, 2026

View reviewed changes

markelov208 added 2 commits January 6, 2026 17:47

moved destructor definition to cpp

fecfc4b

moved includes back to headers

f394cd2

markelov208 dismissed WPK4FEM’s stale review via f394cd2 January 6, 2026 16:47

avdg81 reviewed Jan 7, 2026

View reviewed changes

rfaasse approved these changes Jan 7, 2026

View reviewed changes

markelov208 merged commit 007e25c into master Jan 7, 2026
10 checks passed

markelov208 deleted the geo/14046-move-non-template-code-from-h-to-cpp-constitutive branch January 7, 2026 10:25

[GeoMechanicsApplication] Move all non-template code to .cpp files and make sure any .h file does not contain any implementations (constitutive) #14053

[GeoMechanicsApplication] Move all non-template code to .cpp files and make sure any .h file does not contain any implementations (constitutive) #14053

Conversation

markelov208 commented Dec 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rfaasse left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

avdg81 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

WPK4FEM left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

avdg81 left a comment

Choose a reason for hiding this comment

Uh oh!

rfaasse left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

markelov208 commented Dec 9, 2025 •

edited

Loading