-
Notifications
You must be signed in to change notification settings - Fork 22
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
Implement native support for unified containers #447
base: master
Are you sure you want to change the base?
Conversation
035e95d
to
4dc2b67
Compare
eec86f6
to
2606473
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks like the right direction to me
|
||
namespace detail { | ||
|
||
// Note, that `hsize_t` might be `uint64_t` which could with `uint64_t` or |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sigh
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I should learn to write :)
/** | ||
* Provides the API for reading datasets from HDF5 files. | ||
* | ||
* The CRTP requirements are that `Derived` has a method |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need to CRTP vs virtual, and rely on devirtualization? Is this a hot dispatch?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can't use virtual function calls, because of the T& out
. This is usually std::vector<T>& out
but the inner type differs. I don't think this goes away even if we were to switch to T read(...) const
. Therefore, the only way I know to avoid a bit of code duplication is to use CRTP.
Co-authored-by: MikeG <[email protected]>
Unified HDF5 morphology containers internally concatenate the datasets across all morphologies.