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] Removed all unused math utilities #12383

Merged
merged 1 commit into from
May 15, 2024

Conversation

avdg81
Copy link
Contributor

@avdg81 avdg81 commented May 15, 2024

📝 Description
The removal of these unused functions fixes many code smells. Also removed unused/incorrect #includes from the math utilities file. As a result, several other #includes had to be updated as well.

The removal of these unused functions fixes many code smells. Also
removed unused/incorrect `#include`s from the math utilities file. As a
result, several other `#include`s had to be updated as well.
@avdg81 avdg81 added Cleanup GeoMechanics Issues related to the GeoMechanicsApplication labels May 15, 2024
@avdg81 avdg81 requested review from rfaasse, WPK4FEM and markelov208 May 15, 2024 13:01
@avdg81 avdg81 self-assigned this May 15, 2024
Copy link
Contributor

@WPK4FEM WPK4FEM left a comment

Choose a reason for hiding this comment

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

Not used is bad, not there is better.

Copy link
Contributor

@markelov208 markelov208 left a comment

Choose a reason for hiding this comment

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

Anne, very good to remove a lot of unused staff with their code smells!


namespace Kratos
{

class KRATOS_API(GEO_MECHANICS_APPLICATION) GeoMechanicsMathUtilities
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there need to have 'KRATOS_API(GEO_MECHANICS_APPLICATION)' now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are correct that strictly speaking at the moment there is no need to export this class. However, with the upcoming migration to GTest, I believe it is desirable to have this export in place. Otherwise, we'll need to update the GTest branch to account for that. (The unit test would no longer compile on Windows without this export when we start using GTest.) And I wouldn't want to delay that PR since it got so close to being merged ;-)

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it actually is, since it has a header and source file. When everything is in the header it works, but if I remember correctly I got linking errors when I didn't export this. At least we had similar issues when splitting processes into header and sources

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 have tried to remove the export and rebuild the project, and that worked just fine. The export is just for changing the visibility of the function at the DLL level. As long as the function is being called by other functions that are in the same DLL, the export should not be needed. Only when functions that live in other DLLs need to access this function, the visibility becomes important (or else, linker errors will occur).

Copy link
Contributor

Choose a reason for hiding this comment

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

Ahh okay, I think I was confused because in the past we needed to add the export when splitting into cpp and header for the processes, probably because they are also used in python then?

In any case it's necessary for gtest so indeed it doesn't really matter.

Copy link
Contributor

@rfaasse rfaasse left a comment

Choose a reason for hiding this comment

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

Thanks for taking this over!

@avdg81 avdg81 merged commit c9a0a79 into master May 15, 2024
11 checks passed
@avdg81 avdg81 deleted the geo/clean-up-math-utils branch May 15, 2024 14:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Cleanup GeoMechanics Issues related to the GeoMechanicsApplication
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants