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

[Core] Remove unused members from containers #12767

Merged
merged 3 commits into from
Oct 24, 2024

Conversation

matekelemen
Copy link
Contributor

GlobalPointersVector and PointerVector referred to EqualKeyTo objects/members that don't exist.

This hasn't been a problem until now because these classes are templated and the member functions that referenced EqualKeyTo weren't used and so have never been instantiated. However, it seems like recent compiler versions (e.g.: clang19) introduced some static template analysis that catch this error.

* @param r The GlobalPointersVector to compare with.
* @return True if the containers are equal, false otherwise.
*/
bool operator==(const GlobalPointersVector& r) const // nothrow
Copy link
Member

Choose a reason for hiding this comment

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

hi @matekelemen it looks to me that the comparison would make sense (provided it compiles of couse)
wouldn't it be better to implement the EqualKeyTo rather than removing the method?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, I don't see a use case for comparing the contents of entire containers that wouldn't be possible with std::equal, but in any case, I implemented the function then.

@RiccardoRossi
Copy link
Member

oh well ... if others (@KratosMultiphysics/technical-committee ) are in favor of removing no further resistance on my side ...

@matekelemen matekelemen merged commit 0c5d396 into master Oct 24, 2024
11 checks passed
@matekelemen matekelemen deleted the core/remove-unused-members branch October 24, 2024 14:58
@matekelemen
Copy link
Contributor Author

No no it's fine. I doubt anyone'll use it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants