Merge pull request #173 from aldanor/feature/loc-more

Locations info continued + handle mod cleanup + downcasts

Author: aldanor

CHANGELOG.md

@@ -49,10 +49,18 @@
   but must read the entire attribute at once).
 - Added support in `hdf5-sys` for the new functions in `hdf5` `1.10.6` and `1.10.7`.
 - Added support for creating external links on a `Group` with `link_external`.
-- Added `Location` methods: `get_info`, `get_info_by_name`, `loc_type`, and `open_by_token`.
+- Added `Location` methods returning `LocationInfo`: `loc_info`, `loc_info_by_name`.
+- Added `Location` methods returning `LocationType`: `loc_type`, `loc_type_by_name`.
+- Added `Location::open_by_token` that can open any object by its physical address.
 - Added `Group` methods: `iter_visit`, `iter_visit_default`, `get_all_of_type`, `datasets`, `groups`, and `named_datatypes`.
 - Added `Debug` for `Handle`.
 - Added method `try_borrow` on `Handle` for not taking ownership of an HDF5 object.
+- Added `Handle::id_type()` method.
+- Added `ObjectClass::cast()` method (safe counterpart to `cast_unchecked`).
+- Added downcast methods to `Object`: `as_file`, `as_group`, `as_datatype`, `as_dataspace`,
+  `as_dataset`, `as_attr`, `as_location`, `as_container`, `as_plist`.
+- Simplify setting min library version in `FileAccess` plist: `libver_earliest`, `libver_v18`,
+  `libver_v110`, `libver_latest`; getting min library version can be done via `libver`.
 
  ### Changed
 
@@ -83,12 +91,19 @@
 - Handles to `hdf5` identifiers are no longer tracked via a registry and is instead handled by stricter semantics of ownership.
 - The handle to a `File` will not close all objects in a `File` when dropped, but instead uses a weak file close degree. For the old behaviour see `FileCloseDegree::Strong`.
 - Globals no longer creates a `lazy_static` per global.
+- Unsafe `ObjectClass::cast()` has been renamed to `ObjectClass::cast_unchecked()`.
+- Bump `winreg` (Windows only) to 0.10, `pretty_assertions` (dev) to 1.0.
 
 ### Fixed
 
 - A memory leak of handles has been identified and fixed.
 - A race condition on libary initialisation from different threads was identified and fixed.
 
+### Removed
+
+- Free-standing functions `get_id_type`, `is_valid_id`, `is_valid_user_id` have been removed,
+  but they are all accessible via `Handle`.
+
 ## 0.7.1
 
 ### Added

Cargo.toml

@@ -38,7 +38,7 @@ cfg-if = "1.0"
 
 [dev-dependencies]
 paste = "1.0"
-pretty_assertions = "0.7"
+pretty_assertions = "1.0"
 rand = { version = "0.8", features = ["small_rng"] }
 regex = "1.3"
 scopeguard = "1.0"

hdf5-sys/Cargo.toml

@@ -38,7 +38,7 @@ pkg-config = "0.3"
 [target.'cfg(windows)'.build-dependencies]
 serde = "1.0"
 serde_derive = "1.0"
-winreg = { version = "0.8", features = ["serialization-serde"]}
+winreg = { version = "0.10", features = ["serialization-serde"]}
 
 [package.metadata.docs.rs]
 features = ["static", "zlib"]

src/class.rs

@@ -24,8 +24,8 @@ pub trait ObjectClass: Sized {
 
     fn from_id(id: hid_t) -> Result<Self> {
         h5lock!({
-            if Self::is_valid_id_type(get_id_type(id)) {
-                let handle = Handle::try_new(id)?;
+            let handle = Handle::try_new(id)?;
+            if Self::is_valid_id_type(handle.id_type()) {
                 let obj = Self::from_handle(handle);
                 obj.validate().map(|_| obj)
             } else {
@@ -50,17 +50,27 @@ pub trait ObjectClass: Sized {
         &mut *(self as *mut Self).cast::<T>()
     }
 
-    unsafe fn cast<T: ObjectClass>(self) -> T {
+    unsafe fn cast_unchecked<T: ObjectClass>(self) -> T {
         // This method requires you to be 18 years or older to use it
+        // (note: if it wasn't a trait method, it could be marked as const)
         let obj = ptr::read((&self as *const Self).cast());
         mem::forget(self);
         obj
     }
 
+    fn cast<T: ObjectClass>(self) -> Result<T> {
+        let id_type = self.handle().id_type();
+        if Self::is_valid_id_type(id_type) {
+            Ok(unsafe { self.cast_unchecked() })
+        } else {
+            Err(format!("unable to cast {} ({:?}) into {}", Self::NAME, id_type, T::NAME).into())
+        }
+    }
+
     fn debug_fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
         // TODO: this can moved out if/when specialization lands in stable
         h5lock!({
-            if !is_valid_user_id(self.handle().id()) {
+            if !self.handle().is_valid_user_id() {
                 write!(f, "<HDF5 {}: invalid id>", Self::NAME)
             } else if let Some(d) = self.short_repr() {
                 write!(f, "<HDF5 {}: {}>", Self::NAME, d)

src/handle.rs

@@ -1,34 +1,9 @@
+use std::mem;
+
 use hdf5_sys::h5i::{H5I_type_t, H5Idec_ref, H5Iget_ref, H5Iget_type, H5Iinc_ref, H5Iis_valid};
 
 use crate::internal_prelude::*;
 
-pub fn get_id_type(id: hid_t) -> H5I_type_t {
-    h5lock!({
-        let tp = h5lock!(H5Iget_type(id));
-        let valid = id > 0 && tp > H5I_BADID && tp < H5I_NTYPES;
-        if valid {
-            tp
-        } else {
-            H5I_BADID
-        }
-    })
-}
-
-pub(crate) fn refcount(id: hid_t) -> Result<hsize_t> {
-    h5call!(H5Iget_ref(id)).map(|x| x as _)
-}
-
-pub fn is_valid_id(id: hid_t) -> bool {
-    h5lock!({
-        let tp = get_id_type(id);
-        tp > H5I_BADID && tp < H5I_NTYPES
-    })
-}
-
-pub fn is_valid_user_id(id: hid_t) -> bool {
-    h5lock!({ H5Iis_valid(id) == 1 })
-}
-
 /// A handle to an HDF5 object
 #[derive(Debug)]
 pub struct Handle {
@@ -38,25 +13,23 @@ pub struct Handle {
 impl Handle {
     /// Create a handle from object ID, taking ownership of it
     pub fn try_new(id: hid_t) -> Result<Self> {
-        h5lock!({
-            if is_valid_user_id(id) {
-                Ok(Self { id })
-            } else {
-                Err(From::from(format!("Invalid handle id: {}", id)))
-            }
-        })
+        let handle = Self { id };
+        if handle.is_valid_user_id() {
+            Ok(handle)
+        } else {
+            // Drop on an invalid handle could cause closing an unrelated object
+            // in the destructor, hence it's important to prevent the drop here.
+            mem::forget(handle);
+            Err(From::from(format!("Invalid handle id: {}", id)))
+        }
     }
 
     /// Create a handle from object ID by cloning it
     pub fn try_borrow(id: hid_t) -> Result<Self> {
-        h5lock!({
-            if is_valid_user_id(id) {
-                h5call!(H5Iinc_ref(id))?;
-                Ok(Self { id })
-            } else {
-                Err(From::from(format!("Invalid handle id: {}", id)))
-            }
-        })
+        // It's ok to just call try_new() since it may not decref the object
+        let handle = Self::try_new(id)?;
+        handle.incref();
+        Ok(handle)
     }
 
     pub const fn invalid() -> Self {
@@ -69,8 +42,8 @@ impl Handle {
 
     /// Increment the reference count of the handle
     pub fn incref(&self) {
-        if is_valid_user_id(self.id()) {
-            h5lock!(H5Iinc_ref(self.id()));
+        if self.is_valid_user_id() {
+            h5lock!(H5Iinc_ref(self.id));
         }
     }
 
@@ -81,30 +54,42 @@ impl Handle {
     pub fn decref(&self) {
         h5lock!({
             if self.is_valid_id() {
-                H5Idec_ref(self.id());
+                H5Idec_ref(self.id);
             }
         });
     }
 
     /// Returns `true` if the object has a valid unlocked identifier (`false` for pre-defined
     /// locked identifiers like property list classes).
     pub fn is_valid_user_id(&self) -> bool {
-        is_valid_user_id(self.id())
+        h5lock!(H5Iis_valid(self.id)) == 1
     }
 
     pub fn is_valid_id(&self) -> bool {
-        is_valid_id(self.id())
+        matches!(self.id_type(), tp if tp > H5I_BADID && tp < H5I_NTYPES)
     }
 
     /// Return the reference count of the object
     pub fn refcount(&self) -> u32 {
-        refcount(self.id).unwrap_or(0) as _
+        h5call!(H5Iget_ref(self.id)).map(|x| x as _).unwrap_or(0) as _
+    }
+
+    /// Get HDF5 object type as a native enum.
+    pub fn id_type(&self) -> H5I_type_t {
+        if self.id <= 0 {
+            H5I_BADID
+        } else {
+            match h5lock!(H5Iget_type(self.id)) {
+                tp if tp > H5I_BADID && tp < H5I_NTYPES => tp,
+                _ => H5I_BADID,
+            }
+        }
     }
 }
 
 impl Clone for Handle {
     fn clone(&self) -> Self {
-        Self::try_borrow(self.id()).unwrap_or_else(|_| Self::invalid())
+        Self::try_borrow(self.id).unwrap_or_else(|_| Self::invalid())
     }
 }
 

src/hl/attribute.rs

@@ -397,7 +397,6 @@ pub mod attribute_tests {
     pub fn test_missing() {
         with_tmp_file(|file| {
             let _ = file.new_attr::<u32>().shape((1, 2)).create("foo").unwrap();
-
             let missing_result = file.attr("bar");
             assert!(missing_result.is_err());
         })
@@ -407,22 +406,11 @@ pub mod attribute_tests {
     pub fn test_write_read_str() {
         with_tmp_file(|file| {
             let s = VarLenUnicode::from_str("var len foo").unwrap();
-
-            println!("file.new_attribute");
             let attr = file.new_attr::<VarLenUnicode>().shape(()).create("foo").unwrap();
-
-            println!("write attribute");
             attr.as_writer().write_scalar(&s).unwrap();
-
-            println!("get attribute");
             let read_attr = file.attr("foo").unwrap();
-
-            println!("attribute shape");
             assert_eq!(read_attr.shape(), []);
-
-            println!("read attribute");
             let r: VarLenUnicode = read_attr.as_reader().read_scalar().unwrap();
-
             assert_eq!(r, s);
         })
     }
@@ -433,9 +421,7 @@ pub mod attribute_tests {
             let arr1 = arr2(&[[123], [456]]);
             let _attr1 = file.new_attr_builder().with_data(&arr1).create("foo").unwrap();
             let _attr2 = file.new_attr_builder().with_data("string").create("bar").unwrap();
-
             let attr_names = file.attr_names().unwrap();
-
             assert_eq!(attr_names.len(), 2);
             assert!(attr_names.contains(&"foo".to_string()));
             assert!(attr_names.contains(&"bar".to_string()));

src/hl/container.rs

@@ -380,7 +380,7 @@ impl Deref for Container {
 
 impl Container {
     pub(crate) fn is_attr(&self) -> bool {
-        get_id_type(self.id()) == H5I_ATTR
+        self.handle().id_type() == H5I_ATTR
     }
 
     /// Creates a reader wrapper for this dataset/attribute, allowing to

src/hl/dataspace.rs

@@ -208,7 +208,7 @@ mod tests {
 
     #[test]
     fn test_dataspace_err() {
-        assert_err!(Dataspace::from_id(H5I_INVALID_HID), "Invalid dataspace id");
+        assert_err!(Dataspace::from_id(H5I_INVALID_HID), "Invalid handle id");
     }
 
     #[test]

src/hl/file.rs

@@ -485,6 +485,10 @@ pub mod tests {
         })
     }
 
+    fn rc(id: hid_t) -> Result<hsize_t> {
+        h5call!(hdf5_sys::h5i::H5Iget_ref(id)).map(|x| x as _)
+    }
+
     #[test]
     fn test_strong_close() {
         use crate::hl::plist::file_access::FileCloseDegree;
@@ -506,16 +510,16 @@ pub mod tests {
             assert_eq!(file_copy.refcount(), 2);
 
             drop(file);
-            assert_eq!(crate::handle::refcount(fileid).unwrap(), 1);
+            assert_eq!(rc(fileid).unwrap(), 1);
             assert_eq!(group.refcount(), 1);
             assert_eq!(file_copy.refcount(), 1);
 
             h5lock!({
                 // Lock to ensure fileid does not get overwritten
                 let groupid = group.id();
                 drop(file_copy);
-                assert!(crate::handle::refcount(fileid).is_err());
-                assert!(crate::handle::refcount(groupid).is_err());
+                assert!(rc(fileid).is_err());
+                assert!(rc(groupid).is_err());
                 assert!(!group.is_valid());
                 drop(group);
             });
@@ -543,14 +547,14 @@ pub mod tests {
             assert_eq!(file_copy.refcount(), 2);
 
             drop(file);
-            assert_eq!(crate::handle::refcount(fileid).unwrap(), 1);
+            assert_eq!(rc(fileid).unwrap(), 1);
             assert_eq!(group.refcount(), 1);
             assert_eq!(file_copy.refcount(), 1);
 
             h5lock!({
                 // Lock to ensure fileid does not get overwritten
                 drop(file_copy);
-                assert!(crate::handle::refcount(fileid).is_err());
+                assert!(rc(fileid).is_err());
             });
             assert_eq!(group.refcount(), 1);
         });

src/hl/group.rs

@@ -362,7 +362,7 @@ impl Group {
 
     fn get_all_of_type(&self, loc_type: LocationType) -> Result<Vec<Location>> {
         self.iter_visit_default(vec![], |group, name, _info, objects| {
-            if let Ok(info) = group.get_info_by_name(name) {
+            if let Ok(info) = group.loc_info_by_name(name) {
                 if info.loc_type == loc_type {
                     if let Ok(loc) = group.open_by_token(info.token) {
                         objects.push(loc);
@@ -379,19 +379,19 @@ impl Group {
     /// Returns all groups in the group, non-recursively
     pub fn groups(&self) -> Result<Vec<Self>> {
         self.get_all_of_type(LocationType::Group)
-            .map(|vec| vec.into_iter().map(|obj| unsafe { obj.cast() }).collect())
+            .map(|vec| vec.into_iter().map(|obj| unsafe { obj.cast_unchecked() }).collect())
     }
 
     /// Returns all datasets in the group, non-recursively
     pub fn datasets(&self) -> Result<Vec<Dataset>> {
         self.get_all_of_type(LocationType::Dataset)
-            .map(|vec| vec.into_iter().map(|obj| unsafe { obj.cast() }).collect())
+            .map(|vec| vec.into_iter().map(|obj| unsafe { obj.cast_unchecked() }).collect())
     }
 
     /// Returns all named types in the group, non-recursively
     pub fn named_datatypes(&self) -> Result<Vec<Datatype>> {
         self.get_all_of_type(LocationType::NamedDatatype)
-            .map(|vec| vec.into_iter().map(|obj| unsafe { obj.cast() }).collect())
+            .map(|vec| vec.into_iter().map(|obj| unsafe { obj.cast_unchecked() }).collect())
     }
 
     /// Returns the names of all objects in the group, non-recursively.