From a5aeb6cb890091e101367537db9054dc00a92455 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lauren=C8=9Biu=20Nicola?= Date: Wed, 27 Nov 2024 18:01:27 +0200 Subject: [PATCH] Make string conversion fallible --- CHANGES.md | 1 + src/config.rs | 6 +- src/cpl.rs | 25 ++------ src/dataset.rs | 2 +- src/driver.rs | 4 +- src/gcp.rs | 11 ++-- src/lib.rs | 2 +- src/metadata.rs | 9 +-- src/raster/mdarray.rs | 38 ++++++------ src/raster/rasterband.rs | 13 ++--- src/raster/types.rs | 10 +--- src/spatial_ref/srs.rs | 83 +++++++++++---------------- src/utils.rs | 44 ++++++++------ src/vector/defn.rs | 19 +++--- src/vector/feature.rs | 11 ++-- src/vector/geometry.rs | 10 +--- src/vector/layer.rs | 2 +- src/vector/ops/conversions/formats.rs | 11 ++-- src/version.rs | 3 +- 19 files changed, 120 insertions(+), 184 deletions(-) diff --git a/CHANGES.md b/CHANGES.md index 62f63453..a44e2af0 100644 --- 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` instead of `Result` ([#589](https://github.com/georust/gdal/pull/589)) ### Added diff --git a/src/config.rs b/src/config.rs index fa40ee3e..e126c93b 100644 --- a/src/config.rs +++ b/src/config.rs @@ -55,7 +55,7 @@ pub fn get_config_option(key: &str, default: &str) -> Result { 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) -> Resultinsensitive 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 index 27ce4cb0..f30932ce 100644 --- 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 index 4e0b7e4e..b14dc138 100644 --- 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 index 5e7fe48c..e1aedfb7 100644 --- 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 { 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 index 753a1925..aa5a8626 100644 --- 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 index 9882440d..777b480a 100644 --- a/src/metadata.rs +++ b/src/metadata.rs @@ -59,10 +59,7 @@ pub trait Metadata: MajorObject { /// ``` fn description(&self) -> Result { 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 index 23bcc866..faa5fa25 100644 --- 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 { @@ -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 { diff --git a/src/raster/rasterband.rs b/src/raster/rasterband.rs index 8e652e58..2e47e259 100644 --- 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 index b99a3a33..c5567fc1 100644 --- 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**. diff --git a/src/spatial_ref/srs.rs b/src/spatial_ref/srs.rs index ef819278..4669c93b 100644 --- a/src/spatial_ref/srs.rs +++ b/src/spatial_ref/srs.rs @@ -154,7 +154,7 @@ impl SpatialRef { method_name: "OSRExportToWkt", }) } else { - Ok(_string(c_wkt)) + Ok(_string(c_wkt).unwrap_or_default()) }; unsafe { gdal_sys::VSIFree(c_wkt.cast::()) }; res @@ -180,7 +180,7 @@ impl SpatialRef { method_name: "OSRExportToPrettyWkt", }) } else { - Ok(_string(c_wkt)) + Ok(_string(c_wkt).unwrap_or_default()) }; unsafe { gdal_sys::VSIFree(c_wkt.cast::()) }; res @@ -195,7 +195,7 @@ impl SpatialRef { method_name: "OSRExportToXML", }) } else { - Ok(_string(c_raw_xml)) + Ok(_string(c_raw_xml).unwrap_or_default()) }; unsafe { gdal_sys::VSIFree(c_raw_xml.cast::()) }; res @@ -210,7 +210,7 @@ impl SpatialRef { method_name: "OSRExportToProj4", }) } else { - Ok(_string(c_proj4str)) + Ok(_string(c_proj4str).unwrap_or_default()) }; unsafe { gdal_sys::VSIFree(c_proj4str.cast::()) }; res @@ -226,41 +226,41 @@ impl SpatialRef { method_name: "OSRExportToPROJJSON", }) } else { - Ok(_string(c_projjsonstr)) + Ok(_string(c_projjsonstr).unwrap_or_default()) }; unsafe { gdal_sys::VSIFree(c_projjsonstr.cast::()) }; res } - pub fn auth_name(&self) -> Result { + pub fn auth_name(&self) -> Option { let c_ptr = unsafe { gdal_sys::OSRGetAuthorityName(self.0, ptr::null()) }; - if c_ptr.is_null() { - Err(_last_null_pointer_err("SRGetAuthorityName")) - } else { - Ok(_string(c_ptr)) - } + _string(c_ptr) } pub fn auth_code(&self) -> Result { + // FIXME: this should actually return a string + + let err = GdalError::OgrError { + err: OGRErr::OGRERR_UNSUPPORTED_SRS, + method_name: "OSRGetAuthorityCode", + }; let c_ptr = unsafe { gdal_sys::OSRGetAuthorityCode(self.0, ptr::null()) }; if c_ptr.is_null() { - return Err(_last_null_pointer_err("OSRGetAuthorityCode")); + return Err(err); } + // SAFETY: `c_ptr` can't be null let c_str = unsafe { CStr::from_ptr(c_ptr) }; let epsg = i32::from_str(c_str.to_str()?); match epsg { Ok(n) => Ok(n), - Err(_) => Err(GdalError::OgrError { - err: OGRErr::OGRERR_UNSUPPORTED_SRS, - method_name: "OSRGetAuthorityCode", - }), + Err(_) => Err(err), } } pub fn authority(&self) -> Result { let c_ptr = unsafe { gdal_sys::OSRGetAuthorityName(self.0, ptr::null()) }; if c_ptr.is_null() { - return Err(_last_null_pointer_err("SRGetAuthorityName")); + return Err(_last_null_pointer_err("OSRGetAuthorityName")); } let name = unsafe { CStr::from_ptr(c_ptr) }.to_str()?; let c_ptr = unsafe { gdal_sys::OSRGetAuthorityCode(self.0, ptr::null()) }; @@ -283,34 +283,25 @@ impl SpatialRef { } } - pub fn name(&self) -> Result { + pub fn name(&self) -> Option { let c_ptr = unsafe { gdal_sys::OSRGetName(self.0) }; - if c_ptr.is_null() { - return Err(_last_null_pointer_err("OSRGetName")); - } - Ok(_string(c_ptr)) + _string(c_ptr) } - pub fn angular_units_name(&self) -> Result { + pub fn angular_units_name(&self) -> Option { let mut c_ptr = ptr::null_mut(); unsafe { gdal_sys::OSRGetAngularUnits(self.0, &mut c_ptr) }; - if c_ptr.is_null() { - return Err(_last_null_pointer_err("OSRGetAngularUnits")); - } - Ok(_string(c_ptr)) + _string(c_ptr) } pub fn angular_units(&self) -> f64 { unsafe { gdal_sys::OSRGetAngularUnits(self.0, ptr::null_mut()) } } - pub fn linear_units_name(&self) -> Result { + pub fn linear_units_name(&self) -> Option { let mut c_ptr = ptr::null_mut(); unsafe { gdal_sys::OSRGetLinearUnits(self.0, &mut c_ptr) }; - if c_ptr.is_null() { - return Err(_last_null_pointer_err("OSRGetLinearUnits")); - } - Ok(_string(c_ptr)) + _string(c_ptr) } pub fn linear_units(&self) -> f64 { @@ -378,7 +369,7 @@ impl SpatialRef { } pub fn axis_name(&self, target_key: &str, axis: i32) -> Result { - // See get_axis_orientation + // See axis_orientation let c_ptr = unsafe { gdal_sys::OSRGetAxis( self.0, @@ -387,14 +378,10 @@ impl SpatialRef { ptr::null_mut(), ) }; - if c_ptr.is_null() { - Err(GdalError::AxisNotFoundError { - key: target_key.into(), - method_name: "OSRGetAxis", - }) - } else { - Ok(_string(c_ptr)) - } + _string(c_ptr).ok_or_else(|| GdalError::AxisNotFoundError { + key: target_key.into(), + method_name: "OSRGetAxis", + }) } pub fn axes_count(&self) -> i32 { @@ -450,7 +437,7 @@ impl SpatialRef { south_lat_degree: s_lat, east_lon_degree: e_long, north_lat_degree: n_lat, - name: _string(c_area_name), + name: _string(c_area_name).unwrap_or_default(), }) } else { None @@ -576,12 +563,8 @@ impl SpatialRef { let child = child.try_into().expect("`child` must fit in `c_int`"); let c_node_path = CString::new(node_path)?; - let rv = unsafe { gdal_sys::OSRGetAttrValue(self.0, c_node_path.as_ptr(), child) }; - if rv.is_null() { - Ok(None) - } else { - Ok(Some(_string(rv))) - } + let c_ptr = unsafe { gdal_sys::OSRGetAttrValue(self.0, c_node_path.as_ptr(), child) }; + Ok(_string(c_ptr)) } /// Make a duplicate of the `GEOGCS` node of this [`SpatialRef`]. @@ -740,14 +723,14 @@ mod tests { assert_eq!(spatial_ref.auth_code().unwrap(), 4326); assert_eq!(spatial_ref.authority().unwrap(), "EPSG:4326".to_string()); let spatial_ref = SpatialRef::from_wkt("GEOGCS[\"WGS 84\",DATUM[\"WGS_1984\",SPHEROID[\"WGS 84\",6378137,298.257223563,AUTHORITY[\"EPSG\",7030]],TOWGS84[0,0,0,0,0,0,0],AUTHORITY[\"EPSG\",6326]],PRIMEM[\"Greenwich\",0,AUTHORITY[\"EPSG\",8901]],UNIT[\"DMSH\",0.0174532925199433,AUTHORITY[\"EPSG\",9108]],AXIS[\"Lat\",NORTH],AXIS[\"Long\",EAST]]").unwrap(); - assert!(spatial_ref.auth_name().is_err()); + assert!(spatial_ref.auth_name().is_none()); assert!(spatial_ref.auth_code().is_err()); assert!(spatial_ref.authority().is_err()); let spatial_ref = SpatialRef::from_proj4( "+proj=laea +lat_0=52 +lon_0=10 +x_0=4321000 +y_0=3210000 +ellps=GRS80 +units=m +no_defs", ) .unwrap(); - assert!(spatial_ref.auth_name().is_err()); + assert!(spatial_ref.auth_name().is_none()); assert!(spatial_ref.auth_code().is_err()); assert!(spatial_ref.authority().is_err()); } diff --git a/src/utils.rs b/src/utils.rs index dac642ef..c471f452 100644 --- a/src/utils.rs +++ b/src/utils.rs @@ -5,35 +5,40 @@ use gdal_sys::CPLErr; use crate::errors::*; -pub fn _string(raw_ptr: *const c_char) -> String { - let c_str = unsafe { CStr::from_ptr(raw_ptr) }; - c_str.to_string_lossy().into_owned() -} - -pub(crate) fn _string_tuple(raw_ptr: *const c_char, delim: char) -> Option<(String, String)> { - let c_str = unsafe { CStr::from_ptr(raw_ptr) }; - c_str - .to_string_lossy() - .split_once(delim) - .map(|(k, v)| (k.to_string(), v.to_string())) +/// Makes a copy of a C string, returns `None` for a null pointer. +pub fn _string(raw_ptr: *const c_char) -> Option { + if raw_ptr.is_null() { + None + } else { + let c_str = unsafe { CStr::from_ptr(raw_ptr) }; + Some(c_str.to_string_lossy().into_owned()) + } } +/// Converts an array of C strings to Rust `String`s, skipping any null pointers. pub fn _string_array(raw_ptr: *mut *mut c_char) -> Vec { _convert_raw_ptr_array(raw_ptr, _string) } -pub fn _pathbuf(raw_ptr: *const c_char) -> PathBuf { - let c_str = unsafe { CStr::from_ptr(raw_ptr) }; - c_str.to_string_lossy().into_owned().into() +/// Makes a `PathBuf` from a C string, returns `None` for a null pointer. +pub fn _pathbuf(raw_ptr: *const c_char) -> Option { + if raw_ptr.is_null() { + None + } else { + let c_str = unsafe { CStr::from_ptr(raw_ptr) }; + Some(c_str.to_string_lossy().into_owned().into()) + } } +/// Converts an array of C strings to Rust `PathBuf`s, skipping any null pointers. pub fn _pathbuf_array(raw_ptr: *mut *mut c_char) -> Vec { _convert_raw_ptr_array(raw_ptr, _pathbuf) } +/// Converts an array of C strings, skipping any null pointers. fn _convert_raw_ptr_array(raw_ptr: *mut *mut c_char, convert: F) -> Vec where - F: Fn(*const c_char) -> R, + F: Fn(*const c_char) -> Option, { let mut ret_val = Vec::new(); let mut i = 0; @@ -47,9 +52,10 @@ where if next.is_null() { break; } - let value = convert(next); + if let Some(value) = convert(next) { + ret_val.push(value); + } i += 1; - ret_val.push(value); } } ret_val @@ -63,7 +69,7 @@ pub fn _last_cpl_err(cpl_err_class: CPLErr::Type) -> GdalError { GdalError::CplError { class: cpl_err_class, number: last_err_no, - msg: last_err_msg, + msg: last_err_msg.unwrap_or_default(), } } @@ -72,7 +78,7 @@ pub fn _last_null_pointer_err(method_name: &'static str) -> GdalError { unsafe { gdal_sys::CPLErrorReset() }; GdalError::NullPointer { method_name, - msg: last_err_msg, + msg: last_err_msg.unwrap_or_default(), } } diff --git a/src/vector/defn.rs b/src/vector/defn.rs index a4f08db4..873edda0 100644 --- a/src/vector/defn.rs +++ b/src/vector/defn.rs @@ -73,16 +73,16 @@ impl Defn { /// pub fn field_index>(&self, field_name: S) -> Result { let c_str_field_name = CString::new(field_name.as_ref())?; - let field_id = + let field_idx = unsafe { gdal_sys::OGR_FD_GetFieldIndex(self.c_defn(), c_str_field_name.as_ptr()) }; - if field_id == -1 { + if field_idx == -1 { return Err(GdalError::InvalidFieldName { field_name: field_name.as_ref().to_string(), method_name: "OGR_FD_GetFieldIndex", }); } - let idx = field_id.try_into()?; + let idx = field_idx.try_into()?; Ok(idx) } } @@ -122,13 +122,13 @@ impl<'a> Field<'a> { /// Get the name of this field. pub fn name(&'a self) -> String { let rv = unsafe { gdal_sys::OGR_Fld_GetNameRef(self.c_field_defn) }; - _string(rv) + _string(rv).unwrap_or_default() } /// Get the alternative name (alias) of this field. pub fn alternative_name(&'a self) -> String { let rv = unsafe { gdal_sys::OGR_Fld_GetAlternativeNameRef(self.c_field_defn) }; - _string(rv) + _string(rv).unwrap_or_default() } /// Get the data type of this field. @@ -162,11 +162,8 @@ impl<'a> Field<'a> { /// Get default field value. pub fn default_value(&'a self) -> Option { - let rv = unsafe { gdal_sys::OGR_Fld_GetDefault(self.c_field_defn) }; - if rv.is_null() { - return None; - } - Some(_string(rv)) + let c_ptr = unsafe { gdal_sys::OGR_Fld_GetDefault(self.c_field_defn) }; + _string(c_ptr) } } @@ -206,7 +203,7 @@ impl<'a> GeomField<'a> { /// Get the name of this field. pub fn name(&'a self) -> String { let rv = unsafe { gdal_sys::OGR_GFld_GetNameRef(self.c_field_defn) }; - _string(rv) + _string(rv).unwrap_or_default() } pub fn field_type(&'a self) -> OGRwkbGeometryType::Type { diff --git a/src/vector/feature.rs b/src/vector/feature.rs index 1589ac34..85bb4f55 100644 --- a/src/vector/feature.rs +++ b/src/vector/feature.rs @@ -90,7 +90,7 @@ impl<'a> Feature<'a> { match field_type { OGRFieldType::OFTString => { let rv = unsafe { gdal_sys::OGR_F_GetFieldAsString(self.c_feature, field_idx) }; - Ok(Some(FieldValue::StringValue(_string(rv)))) + Ok(_string(rv).map(FieldValue::StringValue)) } OGRFieldType::OFTStringList => { let rv = unsafe { @@ -268,8 +268,7 @@ impl<'a> Feature<'a> { } let value = _string(unsafe { gdal_sys::OGR_F_GetFieldAsString(self.c_feature, idx) }); - - Ok(Some(value)) + Ok(value) } /// Get the value of the specified field as a [`DateTime`]. @@ -625,13 +624,13 @@ impl Iterator for FieldValueIterator<'_> { let field_defn = unsafe { gdal_sys::OGR_F_GetFieldDefnRef(self.feature.c_feature, idx as c_int) }; let field_name = unsafe { gdal_sys::OGR_Fld_GetNameRef(field_defn) }; - let name = _string(field_name); + let name = _string(field_name).unwrap_or_default(); let fv: Option<(String, Option)> = self .feature .field(idx) .ok() .map(|field_value| (name, field_value)); - //skip unknown types + // skip unknown types if fv.is_none() { return self.next(); } @@ -835,7 +834,7 @@ impl FieldValue { pub fn field_type_to_name(ty: OGRFieldType::Type) -> String { let rv = unsafe { gdal_sys::OGR_GetFieldTypeName(ty) }; - _string(rv) + _string(rv).unwrap_or_default() } #[cfg(test)] diff --git a/src/vector/geometry.rs b/src/vector/geometry.rs index e49ddbcf..224499bf 100644 --- a/src/vector/geometry.rs +++ b/src/vector/geometry.rs @@ -170,11 +170,7 @@ impl Geometry { // Note: C API makes no statements about this possibly returning null. // So we don't have to result wrap this, let c_str = unsafe { gdal_sys::OGR_G_GetGeometryName(self.c_geometry()) }; - if c_str.is_null() { - "".into() - } else { - _string(c_str) - } + _string(c_str).unwrap_or_default() } /// Get the number of elements in a geometry, or number of geometries in container. @@ -358,9 +354,7 @@ impl Eq for Geometry {} pub fn geometry_type_to_name(ty: OGRwkbGeometryType::Type) -> String { let rv = unsafe { gdal_sys::OGRGeometryTypeToName(ty) }; - // If the type is invalid, OGRGeometryTypeToName returns a valid string anyway. - assert!(!rv.is_null()); - _string(rv) + _string(rv).unwrap_or_default() } /// Reference to owned geometry diff --git a/src/vector/layer.rs b/src/vector/layer.rs index 456f031e..5ee1e17e 100644 --- a/src/vector/layer.rs +++ b/src/vector/layer.rs @@ -284,7 +284,7 @@ pub trait LayerAccess: Sized { /// Get the name of this layer. fn name(&self) -> String { let rv = unsafe { gdal_sys::OGR_L_GetName(self.c_layer()) }; - _string(rv) + _string(rv).unwrap_or_default() } fn has_capability(&self, capability: LayerCaps) -> bool { diff --git a/src/vector/ops/conversions/formats.rs b/src/vector/ops/conversions/formats.rs index 512c6136..2c7bc4ba 100644 --- a/src/vector/ops/conversions/formats.rs +++ b/src/vector/ops/conversions/formats.rs @@ -92,8 +92,8 @@ impl Geometry { method_name: "OGR_G_ExportToWkt", }); } - let wkt = _string(c_wkt); - unsafe { gdal_sys::OGRFree(c_wkt as *mut c_void) }; + let wkt = _string(c_wkt).unwrap_or_default(); + unsafe { gdal_sys::VSIFree(c_wkt as *mut c_void) }; Ok(wkt) } @@ -122,12 +122,9 @@ impl Geometry { /// See: [`OGR_G_ExportToJson`](https://gdal.org/api/vector_c_api.html#_CPPv418OGR_G_ExportToJson12OGRGeometryH) pub fn json(&self) -> Result { let c_json = unsafe { gdal_sys::OGR_G_ExportToJson(self.c_geometry()) }; - if c_json.is_null() { - return Err(_last_null_pointer_err("OGR_G_ExportToJson")); - }; - let rv = _string(c_json); + let rv = _string(c_json).ok_or_else(|| _last_null_pointer_err("OGR_G_ExportToJson")); unsafe { gdal_sys::VSIFree(c_json as *mut c_void) }; - Ok(rv) + rv } } diff --git a/src/version.rs b/src/version.rs index e9c8c11f..5c02a5dc 100644 --- a/src/version.rs +++ b/src/version.rs @@ -54,7 +54,8 @@ use std::fmt::Write; /// Details: [`const char *GDALVersionInfo(const char*)`](https://gdal.org/api/raster_c_api.html#_CPPv415GDALVersionInfoPKc) pub fn version_info(key: &str) -> String { let c_key = CString::new(key.as_bytes()).unwrap(); - _string(unsafe { gdal_sys::GDALVersionInfo(c_key.as_ptr()) }) + let rv = unsafe { gdal_sys::GDALVersionInfo(c_key.as_ptr()) }; + _string(rv).unwrap_or_default() } /// Convenience functions for the various pre-defined queryable properties of GDAL version information.