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

Read datatype #796

Merged
merged 3 commits into from Jul 11, 2023
Merged

Read datatype #796

merged 3 commits into from Jul 11, 2023

Conversation

Quark-X10
Copy link
Contributor

Description

HighFive provides the ability to commit datatypes to files with CompoundType::commit for example but does not provide the ability to open the previously commited datatypes.
This PR enhance the Node Traits to get the datatype stored in that node.

How to test this?

cmake ..
make -j8
make test

Test System
-OS: Ubuntu 22.04.2 LTS (WSL)
-Compiler: gcc 11.3.0
-Dependency versions: hdf5 1.10.7

@1uc
Copy link
Collaborator

1uc commented Jul 10, 2023

Thank you, this looks interesting and I'll have a closer look later. In the meantime:

    template <typename Derivate>
    friend class NodeTraits;

is unfortunately a something that's a bit plagued by compiler errors, some compilers require this, while others insist that it's not present. Please see,
https://github.com/BlueBrain/HighFive/blob/master/include/highfive/H5Selection.hpp#L61-L64

for example. If CI complains, please adjust accordingly.

@Quark-X10
Copy link
Contributor Author

Thank you for your quick reply.
I thought the protection was legacy since this type of declaration is unprotected elsewhere like here: https://github.com/BlueBrain/HighFive/blob/master/include/highfive/H5DataSet.hpp#L112-L113
CI does not seems to complain about that but i can correct if needed.
Moreover, I have some trouble to understand why the CI is complaining about a memory leak.

@alkino
Copy link
Member

alkino commented Jul 11, 2023

So the leak is not your responsibility (I think it was only not tested), and this is here: https://github.com/BlueBrain/HighFive/blob/master/include/highfive/H5DataType.hpp#L242
We have to free that.

@codecov
Copy link

codecov bot commented Jul 11, 2023

Codecov Report

Merging #796 (e901735) into master (d2b1173) will increase coverage by 0.04%.
The diff coverage is 94.73%.

@@            Coverage Diff             @@
##           master     #796      +/-   ##
==========================================
+ Coverage   84.21%   84.25%   +0.04%     
==========================================
  Files          68       68              
  Lines        4776     4795      +19     
==========================================
+ Hits         4022     4040      +18     
- Misses        754      755       +1     
Impacted Files Coverage Δ
include/highfive/H5DataType.hpp 89.88% <ø> (ø)
include/highfive/bits/H5Node_traits.hpp 100.00% <ø> (ø)
include/highfive/bits/H5Node_traits_misc.hpp 86.71% <80.00%> (-0.28%) ⬇️
tests/unit/tests_high_five_base.cpp 99.69% <100.00%> (+<0.01%) ⬆️

Copy link
Collaborator

@1uc 1uc left a comment

Choose a reason for hiding this comment

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

Nice work!

@1uc 1uc merged commit 60a3b06 into BlueBrain:master Jul 11, 2023
30 checks passed
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.

None yet

3 participants