From 25f6481f1c1970ca8b1014565cbc970e2a6e1668 Mon Sep 17 00:00:00 2001 From: Nicolas Cornu Date: Fri, 5 Apr 2024 18:36:39 +0200 Subject: [PATCH] Remove deprecated default ctor for DataSet (#947) * Remove deprecated default ctor for DataSet * Allow default ctors. * Test that default constructed objects throw. (#975) The only valid operations of default constructed DataSets and Groups is to assign a valid object to them. Therefore, calling any other methods should raise an exception. --------- Co-authored-by: Luc Grosheintz --- include/highfive/H5DataSet.hpp | 2 - include/highfive/bits/H5Attribute_misc.hpp | 4 + include/highfive/bits/H5DataSet_misc.hpp | 4 + include/highfive/bits/H5Path_traits.hpp | 3 +- include/highfive/bits/H5Path_traits_misc.hpp | 6 +- tests/unit/tests_high_five_base.cpp | 164 +++++++++++++++++-- 6 files changed, 160 insertions(+), 23 deletions(-) diff --git a/include/highfive/H5DataSet.hpp b/include/highfive/H5DataSet.hpp index 0236f06c2..566eb17ff 100644 --- a/include/highfive/H5DataSet.hpp +++ b/include/highfive/H5DataSet.hpp @@ -98,8 +98,6 @@ class DataSet: public Object, return details::get_plist(*this, H5Dget_access_plist); } - /// \deprecated Default constructor creates unsafe uninitialized objects - H5_DEPRECATED("Default constructor creates unsafe uninitialized objects") DataSet() = default; protected: diff --git a/include/highfive/bits/H5Attribute_misc.hpp b/include/highfive/bits/H5Attribute_misc.hpp index 042c63014..19eceb49f 100644 --- a/include/highfive/bits/H5Attribute_misc.hpp +++ b/include/highfive/bits/H5Attribute_misc.hpp @@ -31,6 +31,10 @@ inline std::string Attribute::getName() const { } inline size_t Attribute::getStorageSize() const { + if (!this->isValid()) { + throw AttributeException("Invalid call to `DataSet::getFile` for invalid object"); + } + return static_cast(detail::h5a_get_storage_size(_hid)); } diff --git a/include/highfive/bits/H5DataSet_misc.hpp b/include/highfive/bits/H5DataSet_misc.hpp index 4817fe001..45e530efc 100644 --- a/include/highfive/bits/H5DataSet_misc.hpp +++ b/include/highfive/bits/H5DataSet_misc.hpp @@ -22,6 +22,10 @@ namespace HighFive { inline uint64_t DataSet::getStorageSize() const { + if (!this->isValid()) { + throw DataSetException("Invalid call to `DataSet::getStorageSize` for invalid object"); + } + return detail::h5d_get_storage_size(_hid); } diff --git a/include/highfive/bits/H5Path_traits.hpp b/include/highfive/bits/H5Path_traits.hpp index 46a038c4f..a58f96187 100644 --- a/include/highfive/bits/H5Path_traits.hpp +++ b/include/highfive/bits/H5Path_traits.hpp @@ -25,8 +25,7 @@ class PathTraits { /// /// \brief Return a reference to the File object this object belongs /// \return the File object ref - File& getFile() const noexcept; - + File& getFile() const; protected: std::shared_ptr _file_obj; // keep a ref to file so we keep its ref count > 0 diff --git a/include/highfive/bits/H5Path_traits_misc.hpp b/include/highfive/bits/H5Path_traits_misc.hpp index acde06d1e..0893599c6 100644 --- a/include/highfive/bits/H5Path_traits_misc.hpp +++ b/include/highfive/bits/H5Path_traits_misc.hpp @@ -35,7 +35,11 @@ inline std::string PathTraits::getPath() const { } template -inline File& PathTraits::getFile() const noexcept { +inline File& PathTraits::getFile() const { + const auto& obj = static_cast(*this); + if (!obj.isValid()) { + throw ObjectException("Invalid call to `PathTraits::getFile` for invalid object"); + } return *_file_obj; } diff --git a/tests/unit/tests_high_five_base.cpp b/tests/unit/tests_high_five_base.cpp index dbb6e7fc0..bc33d5dad 100644 --- a/tests/unit/tests_high_five_base.cpp +++ b/tests/unit/tests_high_five_base.cpp @@ -6,6 +6,7 @@ * http://www.boost.org/LICENSE_1_0.txt) * */ +#include #include #include #include @@ -321,27 +322,154 @@ TEST_CASE("Test allocation time") { CHECK(alloc_size == data.size() * sizeof(decltype(data)::value_type)); } -/* - * Test to ensure legacy support: DataSet used to have a default constructor. - * However, it is not useful to have a DataSet object that does not actually - * refer to a dataset in a file. Hence, the the default constructor was - * deprecated. - * This test is to ensure that the constructor is not accidentally removed and - * thereby break users' code. - */ -TEST_CASE("Test default constructors") { - const std::string file_name("h5_default_ctors.h5"); - const std::string dataset_name("dset"); - File file(file_name, File::Truncate); - auto ds = file.createDataSet(dataset_name, std::vector{1, 2, 3, 4, 5}); +template +void check_invalid_hid_Object(T& obj) { + auto silence = SilenceHDF5(); + + CHECK(!obj.isValid()); + CHECK(obj.getId() == H5I_INVALID_HID); + + CHECK_THROWS(obj.getInfo()); + CHECK_THROWS(obj.getType()); +} - DataSet d2; // expect deprecation warning, as it constructs unsafe object - // d2.getFile(); // runtime error - CHECK(!d2.isValid()); - d2 = ds; // copy - CHECK(d2.isValid()); +template +void check_invalid_hid_NodeTraits(T& obj, const U& linkable) { + auto silence = SilenceHDF5(); + + auto data_space = DataSpace{2, 3}; + auto data_type = HighFive::create_datatype(); + auto data = std::vector{1.0, 2.0, 3.0}; + auto gcpl = GroupCreateProps(); + + CHECK_THROWS(obj.createDataSet("foo", data_space, data_type)); + CHECK_THROWS(obj.template createDataSet("foo", data_space)); + CHECK_THROWS(obj.createDataSet("foo", data)); + + CHECK_THROWS(obj.getDataSet("foo")); + CHECK_THROWS(obj.createGroup("foo")); + CHECK_THROWS(obj.createGroup("foo", gcpl)); + CHECK_THROWS(obj.getGroup("foo")); + CHECK_THROWS(obj.getDataType("foo")); + CHECK_THROWS(obj.getNumberObjects()); + CHECK_THROWS(obj.getObjectName(0)); + CHECK_THROWS(obj.rename("foo", "bar")); + CHECK_THROWS(obj.listObjectNames()); + CHECK_THROWS(obj.exist("foo")); + CHECK_THROWS(obj.unlink("foo")); + CHECK_THROWS(obj.getLinkType("foo")); + CHECK_THROWS(obj.getObjectType("foo")); + CHECK_THROWS(obj.createSoftLink("foo", linkable)); + CHECK_THROWS(obj.createSoftLink("foo", "bar")); + CHECK_THROWS(obj.createExternalLink("foo", "bar", "baz")); + CHECK_THROWS(obj.createHardLink("foo", linkable)); } +template +void check_invalid_hid_DataSet(T& obj) { + auto silence = SilenceHDF5(); + + CHECK_THROWS(obj.getStorageSize()); + CHECK_THROWS(obj.getOffset()); + CHECK_THROWS(obj.getMemSpace()); + CHECK_THROWS(obj.resize({1, 2, 3})); + CHECK_THROWS(obj.getDimensions()); + CHECK_THROWS(obj.getElementCount()); + CHECK_THROWS(obj.getCreatePropertyList()); + CHECK_THROWS(obj.getAccessPropertyList()); +} + +template +void check_invalid_hid_SliceTraits(T& obj) { + auto silence = SilenceHDF5(); + + auto slab = HighFive::HyperSlab(RegularHyperSlab({0})); + auto space = DataSpace{3}; + auto set = ElementSet{0, 1, 3}; + auto data = std::vector{1.0, 2.0, 3.0}; + auto type = create_datatype(); + auto cols = std::vector{0, 2, 3}; + + CHECK_THROWS(obj.select(slab)); + CHECK_THROWS(obj.select(slab, space)); + CHECK_THROWS(obj.select({0}, {3})); + CHECK_THROWS(obj.select(cols)); + CHECK_THROWS(obj.select(set)); + + CHECK_THROWS(obj.template read()); + CHECK_THROWS(obj.read(data)); + CHECK_THROWS(obj.read_raw(data.data(), type)); + CHECK_THROWS(obj.template read_raw(data.data())); + + CHECK_THROWS(obj.write(data)); + CHECK_THROWS(obj.write_raw(data.data(), type)); + CHECK_THROWS(obj.template write_raw(data.data())); +} + +template +void check_invalid_hid_PathTraits(T& obj) { + auto silence = SilenceHDF5(); + + CHECK_THROWS(obj.getPath()); + CHECK_THROWS(obj.getFile()); +} + +template +void check_invalid_hid_AnnotateTraits(T& obj) { + auto silence = SilenceHDF5(); + + auto space = DataSpace{3}; + auto data = std::vector{1.0, 2.0, 3.0}; + auto type = create_datatype(); + + CHECK_THROWS(obj.createAttribute("foo", space, type)); + CHECK_THROWS(obj.template createAttribute("foo", space)); + CHECK_THROWS(obj.createAttribute("foo", data)); + + CHECK_THROWS(obj.deleteAttribute("foo")); + CHECK_THROWS(obj.getAttribute("foo")); + CHECK_THROWS(obj.getNumberAttributes()); + CHECK_THROWS(obj.listAttributeNames()); + CHECK_THROWS(obj.hasAttribute("foo")); +} + +template +void check_invalid_hid_Group(T& obj) { + auto silence = SilenceHDF5(); + + CHECK_THROWS(obj.getEstimatedLinkInfo()); + CHECK_THROWS(obj.getCreatePropertyList()); +} + +TEST_CASE("Test default DataSet constructor") { + DataSet ds; + check_invalid_hid_Object(ds); + check_invalid_hid_DataSet(ds); + check_invalid_hid_SliceTraits(ds); + check_invalid_hid_AnnotateTraits(ds); + check_invalid_hid_PathTraits(ds); + + File file("h5_default_dset_ctor.h5", File::Truncate); + ds = file.createDataSet("dset", std::vector{1, 2, 3, 4, 5}); + CHECK(ds.isValid()); +} + +TEST_CASE("Test default Group constructor") { + File file("h5_default_group_ctor.h5", File::Truncate); + Group linkable = file.createGroup("bar"); + + Group grp; + check_invalid_hid_Object(grp); + check_invalid_hid_NodeTraits(grp, linkable); + check_invalid_hid_AnnotateTraits(grp); + check_invalid_hid_PathTraits(grp); + + grp = file.createGroup("grp"); + + CHECK(grp.isValid()); +} + + TEST_CASE("Test groups and datasets") { const std::string file_name("h5_group_test.h5"); const std::string dataset_name("dset");