Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[GeoMechanicsApplication] Split ApplyComponentTableProcess in header and source #13122

Merged

Conversation

rfaasse
Copy link
Contributor

@rfaasse rfaasse commented Feb 12, 2025

📝 Description
There was no reason to have a single .hpp file (no templated functions) for this process, so it was split into a header + source. No functional changes were done, just:

  • Split the class in 2 files
  • Remove some redundant comments
  • [[nodiscard]] added for the Info function

@rfaasse rfaasse requested a review from avdg81 February 12, 2025 15:30
@rfaasse rfaasse self-assigned this Feb 12, 2025
Copy link
Contributor

@avdg81 avdg81 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a straightforward and clear PR. Thank you for splitting the original header into a cleaner header and an implementation file. I have only a few very minor comments.


#include "includes/kratos_export_api.h"
#include "includes/kratos_parameters.h"
#include "includes/model_part.h"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe we can move this #include to the .cpp file now, since the interface uses references to ModelParts only. In other words, a forward declaration will do here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought so to, tried, but get (very vague) compiler errors, let's have a look together at this

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Managed to find why, I added a missing include in a different file to be able to get the forward declare to work


using TableType = Table<double, double>;

ApplyComponentTableProcess(ModelPart& model_part, Parameters rParameters);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's make the parameter names comply with the Kratos Style Guide:

Suggested change
ApplyComponentTableProcess(ModelPart& model_part, Parameters rParameters);
ApplyComponentTableProcess(ModelPart& rModelPart, Parameters ProcessSettings);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@rfaasse rfaasse requested a review from avdg81 February 13, 2025 12:13
Copy link
Contributor

@avdg81 avdg81 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this PR is ready to go.

@rfaasse rfaasse enabled auto-merge (squash) February 13, 2025 14:07
@rfaasse rfaasse merged commit 31e8984 into master Feb 13, 2025
11 checks passed
@rfaasse rfaasse deleted the geo/split-apply-component-table-process-in-header-source branch February 13, 2025 14:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants