Make string conversion fallible

georust · Nov 29, 2024 · a5aeb6c · a5aeb6c
1 parent 9692dae
commit a5aeb6c
Show file tree

Hide file tree

Showing 19 changed files with 120 additions and 184 deletions.
diff --git a/CHANGES.md b/CHANGES.md
@@ -14,6 +14,7 @@
   - Update `Feature::field_count` to return `usize` instead of `i32` ([#581](https://github.com/georust/gdal/pull/581))
   - Update `Feature::field_as_integer`, `Feature::field_as_integer64`, `Feature::field_as_double`, `Feature::field_as_string`, `Feature::field_as_datetime` to take the field index as `usize` instead of `i32` ([#581](https://github.com/georust/gdal/pull/581))
   - Drop `LayerAccess::create_feature_fields` ([#581](https://github.com/georust/gdal/pull/581))
+  - Update `SpatialRef::auth_name`, `SpatialRef::name`, `SpatialRef::angular_units_name`, `SpatialRef::linear_units_name` to return `Option<String>` instead of `Result<String>` ([#589](https://github.com/georust/gdal/pull/589))
 
 ### Added
 

diff --git a/src/config.rs b/src/config.rs
@@ -55,7 +55,7 @@ pub fn get_config_option(key: &str, default: &str) -> Result<String> {
     let c_key = CString::new(key.as_bytes())?;
     let c_default = CString::new(default.as_bytes())?;
     let rv = unsafe { gdal_sys::CPLGetConfigOption(c_key.as_ptr(), c_default.as_ptr()) };
-    Ok(_string(rv))
+    Ok(_string(rv).unwrap_or_else(|| default.to_string()))
 }
 
 /// Clear the value of a GDAL library configuration option
@@ -96,7 +96,7 @@ pub fn get_thread_local_config_option(key: &str, default: &str) -> Result<String
     let c_key = CString::new(key.as_bytes())?;
     let c_default = CString::new(default.as_bytes())?;
     let rv = unsafe { gdal_sys::CPLGetThreadLocalConfigOption(c_key.as_ptr(), c_default.as_ptr()) };
-    Ok(_string(rv))
+    Ok(_string(rv).unwrap_or_else(|| default.to_string()))
 }
 
 /// Clear the value of a GDAL library configuration option
@@ -141,7 +141,7 @@ where
         error_num: CPLErrorNum,
         error_msg_ptr: *const c_char,
     ) {
-        let error_msg = _string(error_msg_ptr);
+        let error_msg = _string(error_msg_ptr).unwrap_or_default();
         let error_type: CplErrType = error_type.into();
 
         // reconstruct callback from user data pointer

diff --git a/src/cpl.rs b/src/cpl.rs
@@ -6,7 +6,6 @@
 use std::ffi::{c_char, c_int, CString};
 use std::fmt::{Debug, Display, Formatter};
 use std::mem::ManuallyDrop;
-use std::ops::Deref;
 use std::ptr;
 use std::str::FromStr;
 
@@ -178,11 +177,7 @@ impl CslStringList {
         // we know already `name` will never exist in a valid CslStringList.
         let key = CString::new(name).ok()?;
         let c_value = unsafe { CSLFetchNameValue(self.as_ptr(), key.as_ptr()) };
-        if c_value.is_null() {
-            None
-        } else {
-            Some(_string(c_value))
-        }
+        _string(c_value)
     }
 
     /// Perform a case <u>insensitive</u> search for the given string
@@ -240,20 +235,13 @@ impl CslStringList {
         if index > c_int::MAX as usize {
             return None;
         }
+
         // In the C++ implementation, an index-out-of-bounds returns an empty string, not an error.
         // We don't want to check against `len` because that scans the list.
         // See: https://github.com/OSGeo/gdal/blob/fada29feb681e97f0fc4e8861e07f86b16855681/port/cpl_string.cpp#L181-L182
         let field = unsafe { CSLGetField(self.as_ptr(), index as c_int) };
-        if field.is_null() {
-            return None;
-        }
 
-        let field = _string(field);
-        if field.is_empty() {
-            None
-        } else {
-            Some(field.deref().into())
-        }
+        _string(field).filter(|s| !s.is_empty()).map(|s| s.into())
     }
 
     /// Determine the number of entries in the list.
@@ -505,12 +493,7 @@ impl Iterator for CslStringListIterator<'_> {
             slice[self.idx]
         };
         self.idx += 1;
-        if field.is_null() {
-            None
-        } else {
-            let entry = _string(field);
-            Some(entry.deref().into())
-        }
+        _string(field).map(|s| s.into())
     }
 }
 

diff --git a/src/dataset.rs b/src/dataset.rs
@@ -235,7 +235,7 @@ impl Dataset {
     /// Fetch the projection definition string for this dataset.
     pub fn projection(&self) -> String {
         let rv = unsafe { gdal_sys::GDALGetProjectionRef(self.c_dataset) };
-        _string(rv)
+        _string(rv).unwrap_or_default()
     }
 
     /// Set the projection reference string for this dataset.

diff --git a/src/driver.rs b/src/driver.rs
@@ -66,7 +66,7 @@ impl Driver {
     /// See also: [`long_name`](Self::long_name).
     pub fn short_name(&self) -> String {
         let rv = unsafe { gdal_sys::GDALGetDriverShortName(self.c_driver) };
-        _string(rv)
+        _string(rv).unwrap_or_default()
     }
 
     /// Return the short name of a driver.
@@ -76,7 +76,7 @@ impl Driver {
     /// See also: [`short_name`](Self::short_name`).
     pub fn long_name(&self) -> String {
         let rv = unsafe { gdal_sys::GDALGetDriverLongName(self.c_driver) };
-        _string(rv)
+        _string(rv).unwrap_or_default()
     }
 
     /// Create a new dataset of size (`size_x`, `size_y`) and `bands` band count,

diff --git a/src/gcp.rs b/src/gcp.rs
@@ -81,8 +81,10 @@ impl GcpRef<'_> {
 impl From<&gdal_sys::GDAL_GCP> for Gcp {
     fn from(gcp: &gdal_sys::GDAL_GCP) -> Self {
         Gcp {
-            id: _string(gcp.pszId),
-            info: _string(gcp.pszId),
+            // This seems to never be NULL
+            id: _string(gcp.pszId).unwrap_or_default(),
+            // GDAL docs state that `pszInfo` is filled in or `""`
+            info: _string(gcp.pszInfo).unwrap_or_default(),
             pixel: gcp.dfGCPPixel,
             line: gcp.dfGCPLine,
             x: gcp.dfGCPX,
@@ -133,10 +135,7 @@ impl Dataset {
     ///  See: [`GDALGetGCPProjection`](https://gdal.org/api/raster_c_api.html#gdal_8h_1a85ffa184d3ecb7c0a59a66096b22b2ec)
     pub fn gcp_projection(&self) -> Option<String> {
         let cc_ptr = unsafe { gdal_sys::GDALGetGCPProjection(self.c_dataset()) };
-        if cc_ptr.is_null() {
-            return None;
-        }
-        Some(_string(cc_ptr))
+        _string(cc_ptr)
     }
 
     /// Fetch GCPs.

diff --git a/src/lib.rs b/src/lib.rs
@@ -47,7 +47,7 @@
 //! use gdal::Dataset;
 //! # fn main() -> gdal::errors::Result<()> {
 //! let ds = Dataset::open("fixtures/m_3607824_se_17_1_20160620_sub.tif")?;
-//! println!("This {} is in '{}' and has {} bands.", ds.driver().long_name(), ds.spatial_ref()?.name()?, ds.raster_count());
+//! println!("This {} is in '{}' and has {} bands.", ds.driver().long_name(), ds.spatial_ref()?.name().unwrap(), ds.raster_count());
 //! # Ok(())
 //! # }
 //! ```

diff --git a/src/metadata.rs b/src/metadata.rs
@@ -59,10 +59,7 @@ pub trait Metadata: MajorObject {
     /// ```
     fn description(&self) -> Result<String> {
         let c_res = unsafe { gdal_sys::GDALGetDescription(self.gdal_object_ptr()) };
-        if c_res.is_null() {
-            return Err(_last_null_pointer_err("GDALGetDescription"));
-        }
-        Ok(_string(c_res))
+        _string(c_res).ok_or_else(|| _last_null_pointer_err("GDALGetDescription"))
     }
 
     /// Metadata in GDAL is partitioned into namespaces, knows as "domains" in the
@@ -150,9 +147,7 @@ pub trait Metadata: MajorObject {
                         c_domain.as_ptr(),
                     )
                 };
-                if !c_res.is_null() {
-                    return Some(_string(c_res));
-                }
+                return _string(c_res);
             }
         }
         None

diff --git a/src/raster/mdarray.rs b/src/raster/mdarray.rs
@@ -331,16 +331,11 @@ impl<'a> MDArray<'a> {
 
             let strings = string_pointers
                 .into_iter()
-                .map(|string_ptr| {
-                    if string_ptr.is_null() {
-                        String::new()
-                    } else {
-                        let string = _string(string_ptr);
-
-                        VSIFree(string_ptr as *mut c_void);
-
-                        string
-                    }
+                .filter_map(|ptr| {
+                    let s = _string(ptr);
+                    // GDAL allows `VSIFree(nullptr)`
+                    VSIFree(ptr as *mut c_void);
+                    s
                 })
                 .collect();
 
@@ -376,9 +371,8 @@ impl<'a> MDArray<'a> {
     pub fn unit(&self) -> String {
         unsafe {
             // should not be freed
-            let c_unit = GDALMDArrayGetUnit(self.c_mdarray);
-
-            _string(c_unit)
+            let c_ptr = GDALMDArrayGetUnit(self.c_mdarray);
+            _string(c_ptr).unwrap_or_default()
         }
     }
 
@@ -488,7 +482,8 @@ impl<'a> Group<'a> {
     }
 
     pub fn name(&self) -> String {
-        _string(unsafe { GDALGroupGetName(self.c_group) })
+        let c_ptr = unsafe { GDALGroupGetName(self.c_group) };
+        _string(c_ptr).unwrap_or_default()
     }
 
     pub fn group_names(&self, options: CslStringList) -> Vec<String> {
@@ -624,7 +619,8 @@ impl<'a> Dimension<'a> {
     }
 
     pub fn name(&self) -> String {
-        _string(unsafe { GDALDimensionGetName(self.c_dimension) })
+        let c_ptr = unsafe { GDALDimensionGetName(self.c_dimension) };
+        _string(c_ptr).unwrap_or_default()
     }
 
     pub fn indexing_variable(&self) -> MDArray {
@@ -712,7 +708,8 @@ impl ExtendedDataType {
     }
 
     pub fn name(&self) -> String {
-        _string(unsafe { GDALExtendedDataTypeGetName(self.c_data_type) })
+        let c_ptr = unsafe { GDALExtendedDataTypeGetName(self.c_data_type) };
+        _string(c_ptr).unwrap_or_default()
     }
 }
 
@@ -767,12 +764,9 @@ impl Attribute {
     }
 
     pub fn read_as_string(&self) -> String {
-        unsafe {
-            // SAFETY: should no be freed
-            let c_string = GDALAttributeReadAsString(self.c_attribute);
-
-            _string(c_string)
-        }
+        // should not be freed
+        let c_ptr = unsafe { GDALAttributeReadAsString(self.c_attribute) };
+        _string(c_ptr).unwrap_or_default()
     }
 
     pub fn read_as_string_array(&self) -> Vec<String> {

diff --git a/src/raster/rasterband.rs b/src/raster/rasterband.rs
@@ -950,13 +950,8 @@ impl<'a> RasterBand<'a> {
     /// Return the unit of the rasterband.
     /// If there is no unit, the empty string is returned.
     pub fn unit(&self) -> String {
-        let str_ptr = unsafe { gdal_sys::GDALGetRasterUnitType(self.c_rasterband) };
-
-        if str_ptr.is_null() {
-            return String::new();
-        }
-
-        _string(str_ptr)
+        let c_ptr = unsafe { gdal_sys::GDALGetRasterUnitType(self.c_rasterband) };
+        _string(c_ptr).unwrap_or_default()
     }
 
     /// Read the band mask flags for a GDAL `RasterBand`.
@@ -1362,8 +1357,8 @@ impl ColorInterpretation {
 
     /// Returns the name of this color interpretation.
     pub fn name(&self) -> String {
-        let rv = unsafe { gdal_sys::GDALGetColorInterpretationName(self.c_int()) };
-        _string(rv)
+        let c_ptr = unsafe { gdal_sys::GDALGetColorInterpretationName(self.c_int()) };
+        _string(c_ptr).unwrap_or_default()
     }
 }
 

diff --git a/src/raster/types.rs b/src/raster/types.rs
@@ -103,15 +103,7 @@ impl GdalDataType {
     /// ```
     pub fn name(&self) -> String {
         let c_str = unsafe { GDALGetDataTypeName(self.gdal_ordinal()) };
-        if c_str.is_null() {
-            // This case shouldn't happen, because `self` only exists for valid
-            // GDALDataType ordinals.
-            panic!(
-                "GDALGetDataTypeName unexpectedly returned an empty name for {:?}",
-                &self
-            );
-        }
-        _string(c_str)
+        _string(c_str).unwrap_or_default()
     }
 
     /// Get the [`GDALDataType`] size in **bits**.