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

Move ItemCollection and derived classes to core #1443

Merged
merged 8 commits into from
Oct 30, 2024

Conversation

nselliott
Copy link
Contributor

Summary

  • This PR is a refactoring
  • It does the following:
    • Moves the ItemCollection and its child classes from Sidre to core, to enable them to be used without depending on the Sidre component.
    • Fixes Move Sidre collections to core #1397 , may be useful within Sina

Moving these header-only classes required removing their dependencies on Slic, as there were a few uses of Slic assertions and warnings. Those have been replaced with raw assert() calls and output to std::cerr, matching what is done elsewhere in Axom core.

The definitions of constants InvalidIndex and InvalidName needed to be moved from Sidre into core for usage by the collection classes.

Copy link
Member

@rhornung67 rhornung67 left a comment

Choose a reason for hiding this comment

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

Thanks @nselliott

Copy link
Member

@kennyweiss kennyweiss left a comment

Choose a reason for hiding this comment

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

Thanks @nselliott

Please update the RELEASE_NOTES with this change.
Should add some unit tests to core for basic operations on these classes?

@@ -222,7 +220,7 @@ class MapCollection : public ItemCollection<T>
const std::string& getItemName(IndexType idx) const
{
return (hasItem(idx) ? m_items[static_cast<unsigned>(idx)]->getName()
Copy link
Member

Choose a reason for hiding this comment

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

@nselliott -- This function requires the templated type T to have a getName() function.

Could you please document this requirement?
Alternatively, is there a way to conditionally include this function via enable_if ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This requirement is documented in the header. I think it's better not to add a SFINAE implementation, as the core feature of MapCollection is to hold a collection of named items. Using enable_if would make the code more complicated and allow instances of MapCollection to compile that don't have a meaningful use case, so in my opinion it's better to catch attempts to do that at compile time.

@nselliott
Copy link
Contributor Author

Thanks @nselliott

Please update the RELEASE_NOTES with this change. Should add some unit tests to core for basic operations on these classes?

The existing set of unit tests for these classes in the sidre test directory is still good. We could move them to the core test directory if needed.

@nselliott nselliott merged commit e5b5707 into develop Oct 30, 2024
13 checks passed
@nselliott nselliott deleted the feature/nselliott/core-collections branch October 30, 2024 21:50
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.

Move Sidre collections to core
3 participants