[alt to #281] Isolate hdf5 as a dependency of Cello's disk component

[jobordner]

Alternative to PR #281 to relocate #include "hdf5.h" from _disk.hpp to disk_FileHdf5.cpp.

[bwoshea]

@jobordner could you please give a bit more context? Should we decline PR #281 and go with this one, or does that require some discussion?

[mabruzzo]

I think this looks great! I'll be happy to close 281.

My 1 suggestion is that we declare a type alias called something h5_int (an alias of int64_t) or something and use that instead of int64_t (so that it's more clear why we are passing in int64 values to these functions.

[mabruzzo]

Right, so as I said just above this looks great to me! I've closed #281.

There are 2 things I would recommend changing. Let me know if you disagree with any of the suggestions. Then I would be happy to approve the PR.

The suggestions include:

1. Rather than using int64_t or hid as the argument and return type of all of the FileHdf5 methods, I would define a custom type (say h5_int_t) that aliases int64_t and use that instead.

   • To be a little more concrete, here's what I'm (roughly) suggesting:

     1. add a line like the following

        // this type is identical to the hdf5 library's `hid` type. We define this type
        // here, so that we can explicitly avoid including <hdf5.h> in any headers
        typedef int64_t h5_int_t;

        to either src/Cello/_disk.hpp or to the top of src/Cello/disk_FileHdf5.hpp.
     2. replace all occurrences of int64_t in src/Cello/_disk.[ch]pp with h5_int_t

   • I think making this change will help clarify our intent for future generations. (Back when when I first read this code, I was unfamiliar with how the hdf5 library worked. If we were simply passing around int64_t rather than hid, I would have been fairly confused).

2. Less importantly, it might be nice to make a minor tweak to src/Cello/CMakeLists.txt to indicate that other components don't need to link against the hdf5 library.

   • I would suggest replacing the following lines

     # with minimal refactoring, we could make HDF5_C a privately linked library
     target_link_libraries(disk
       PUBLIC HDF5_C
       PRIVATE pngwriter
       PUBLIC error
       )

     with this snippet

     target_link_libraries(disk
       PRIVATE HDF5_C
       PRIVATE pngwriter
       PUBLIC error
       )

     (essentially, you would remove the comment and make the HDF5_C library into a private dependency of the disk component)

   • Definitely feel free to punt on this. I could always handle this later.

[jwise77]

We've decided that this PR can be merged with 1 approval.

[jobordner]

Right, so as I said just above this looks great to me! I've closed #281.

There are 2 things I would recommend changing. Let me know if you disagree with any of the suggestions. Then I would be happy to approve the PR.

The suggestions include:

1. Rather than using int64_t or hid as the argument and return type of all of the FileHdf5 methods, I would define a custom type (say h5_int_t) that aliases int64_t and use that instead.

   • To be a little more concrete, here's what I'm (roughly) suggesting:

     1. add a line like the following

        // this type is identical to the hdf5 library's `hid` type. We define this type
        // here, so that we can explicitly avoid including <hdf5.h> in any headers
        typedef int64_t h5_int_t;

        to either src/Cello/_disk.hpp or to the top of src/Cello/disk_FileHdf5.hpp.

     2. replace all occurrences of int64_t in src/Cello/_disk.[ch]pp with h5_int_t

   • I think making this change will help clarify our intent for future generations. (Back when when I first read this code, I was unfamiliar with how the hdf5 library worked. If we were simply passing around int64_t rather than hid, I would have been fairly confused).

2. Less importantly, it might be nice to make a minor tweak to src/Cello/CMakeLists.txt to indicate that other components don't need to link against the hdf5 library.

   • I would suggest replacing the following lines

     # with minimal refactoring, we could make HDF5_C a privately linked library
     target_link_libraries(disk
       PUBLIC HDF5_C
       PRIVATE pngwriter
       PUBLIC error
       )

     with this snippet

     target_link_libraries(disk
       PRIVATE HDF5_C
       PRIVATE pngwriter
       PUBLIC error
       )

     (essentially, you would remove the comment and make the HDF5_C library into a private dependency of the disk component)

   • Definitely feel free to punt on this. I could always handle this later.

I've made the above changes, using the name "hdf5_id" (I also removed the former int64_t typed attribute data_dims_ because I noticed it's never accessed). Thanks for the feedback!

[mabruzzo]

Thanks for making the changes! Let's merge once the tests pass!