From b600112278ef7e27274a185ba83d9dab72f305d8 Mon Sep 17 00:00:00 2001 From: "Simeon H.K. Fitch" Date: Tue, 20 Sep 2022 13:08:23 -0400 Subject: [PATCH 1/5] Quality-of-life additions to `CplStringList`. --- CHANGES.md | 4 ++ src/cpl.rs | 160 ++++++++++++++++++++++++++++++++++++++++++++++++--- src/utils.rs | 8 +++ 3 files changed, 163 insertions(+), 9 deletions(-) diff --git a/CHANGES.md b/CHANGES.md index adcae2047..ef35727d2 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -19,6 +19,10 @@ - +- Added quality-of-life features to `CslStringList`: `len`, `is_empty`, `Debug` and `Iterator` implementations. + + - <> + ## 0.13 - Add prebuild bindings for GDAL 3.5 diff --git a/src/cpl.rs b/src/cpl.rs index 93278b43f..df0353e9f 100644 --- a/src/cpl.rs +++ b/src/cpl.rs @@ -4,18 +4,18 @@ //! use std::ffi::CString; +use std::fmt::{Debug, Formatter}; use std::ptr; -use gdal_sys::CSLSetNameValue; +use gdal_sys::{CSLCount, CSLDestroy, CSLFetchNameValue, CSLGetField, CSLSetNameValue}; use libc::c_char; use crate::errors::{GdalError, Result}; -use crate::utils::_string; +use crate::utils::{_string, _string_tuple}; -/// Wraps a `char **papszStrList` pointer into a struct that -/// automatically destroys the allocated memory on `drop`. +/// Wraps a [`gdal_sys::CSLConstList`] (a.k.a. `char **papszStrList`) /// -/// See the `CSL` GDAL functions for more details. +/// See the [`CSL*` GDAL functions](https://gdal.org/api/cpl.html#cpl-string-h) for more details. pub struct CslStringList { list_ptr: *mut *mut c_char, } @@ -30,6 +30,9 @@ impl CslStringList { /// Assigns `value` to `name`. /// /// Overwrites duplicate `name`s. + /// + /// Returns `Ok<()>` on success, `Err` if `name` has non alphanumeric + /// characters, or `value` has newline characters. pub fn set_name_value(&mut self, name: &str, value: &str) -> Result<()> { if !name.chars().all(|c| c.is_ascii_alphanumeric() || c == '_') { return Err(GdalError::BadArgument(format!( @@ -53,12 +56,13 @@ impl CslStringList { Ok(()) } - /// Looks up the value corresponding to a key. + /// Looks up the value corresponding to `key`. /// - /// See `CSLFetchNameValue` for details. + /// See [`CSLFetchNameValue`](https://gdal.org/doxygen/cpl__string_8h.html#a4f23675f8b6f015ed23d9928048361a1) + /// for details. pub fn fetch_name_value(&self, key: &str) -> Result> { let key = CString::new(key)?; - let c_value = unsafe { gdal_sys::CSLFetchNameValue(self.as_ptr(), key.as_ptr()) }; + let c_value = unsafe { CSLFetchNameValue(self.as_ptr(), key.as_ptr()) }; let value = if c_value.is_null() { None } else { @@ -67,6 +71,22 @@ impl CslStringList { Ok(value) } + /// Determine the number of entries in the list. + pub fn len(&self) -> isize { + unsafe { CSLCount(self.as_ptr()) as isize } + } + + /// Determine if the list has any values + pub fn is_empty(&self) -> bool { + self.len() <= 0 + } + + /// Get an iterator over the name/value elements of the list. + pub fn iter(&self) -> CslStringListIterator { + CslStringListIterator::new(self) + } + + /// Get the raw pointer behind list's data. pub fn as_ptr(&self) -> gdal_sys::CSLConstList { self.list_ptr } @@ -74,7 +94,7 @@ impl CslStringList { impl Drop for CslStringList { fn drop(&mut self) { - unsafe { gdal_sys::CSLDestroy(self.list_ptr) } + unsafe { CSLDestroy(self.list_ptr) } } } @@ -83,3 +103,125 @@ impl Default for CslStringList { Self::new() } } + +pub struct CslStringListIterator<'a> { + list: &'a CslStringList, + idx: isize, + count: isize, +} + +impl<'a> CslStringListIterator<'a> { + fn new(list: &'a CslStringList) -> Self { + Self { + list, + idx: 0, + count: list.len(), + } + } + fn is_done(&self) -> bool { + self.idx >= self.count + } +} + +impl<'a> Iterator for CslStringListIterator<'a> { + type Item = (String, String); + + fn next(&mut self) -> Option { + if self.is_done() { + return None; + } + + let field = unsafe { CSLGetField(self.list.as_ptr(), self.idx as libc::c_int) }; + if field.is_null() { + None + } else { + self.idx += 1; + _string_tuple(field, '=') + } + } +} + +impl Debug for CslStringList { + fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { + for (k, v) in self.iter() { + f.write_fmt(format_args!("{k}={v}\n"))?; + } + Ok(()) + } +} + +#[cfg(test)] +mod tests { + use crate::cpl::CslStringList; + use crate::errors::Result; + + fn fixture() -> Result { + let mut l = CslStringList::new(); + l.set_name_value("ONE", "1")?; + l.set_name_value("TWO", "2")?; + l.set_name_value("THREE", "3")?; + + Ok(l) + } + + #[test] + fn basic_list() -> Result<()> { + let l = fixture()?; + assert!(matches!(l.fetch_name_value("ONE"), Ok(Some(s)) if s == *"1")); + assert!(matches!(l.fetch_name_value("THREE"), Ok(Some(s)) if s == *"3")); + assert!(matches!(l.fetch_name_value("FOO"), Ok(None))); + + Ok(()) + } + + #[test] + fn has_length() -> Result<()> { + let l = fixture()?; + assert_eq!(l.len(), 3); + + Ok(()) + } + + #[test] + fn can_be_empty() -> Result<()> { + let l = CslStringList::new(); + assert!(l.is_empty()); + + let l = fixture()?; + assert!(!l.is_empty()); + + Ok(()) + } + + #[test] + fn has_iterator() -> Result<()> { + let f = fixture()?; + let mut it = f.iter(); + assert_eq!(it.next(), Some(("ONE".to_string(), "1".to_string()))); + assert_eq!(it.next(), Some(("TWO".to_string(), "2".to_string()))); + assert_eq!(it.next(), Some(("THREE".to_string(), "3".to_string()))); + assert_eq!(it.next(), None); + assert_eq!(it.next(), None); + Ok(()) + } + + #[test] + fn invalid_keys() -> Result<()> { + let mut l = fixture()?; + assert!(l.set_name_value("l==t", "2").is_err()); + assert!(l.set_name_value("foo", "2\n4\r5").is_err()); + + Ok(()) + } + + #[test] + fn debug_fmt() -> Result<()> { + let l = fixture()?; + let s = format!("{l:?}"); + assert!(s.contains("ONE=1")); + assert!(s.contains("TWO=2")); + assert!(s.contains("THREE=3")); + + Ok(()) + } +} diff --git a/src/utils.rs b/src/utils.rs index ff19695b4..7670dc7ef 100644 --- a/src/utils.rs +++ b/src/utils.rs @@ -10,6 +10,14 @@ pub fn _string(raw_ptr: *const c_char) -> String { 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())) +} + pub fn _string_array(raw_ptr: *mut *mut c_char) -> Vec { _convert_raw_ptr_array(raw_ptr, _string) } From 43d126dae19cebb15d1c4ea6a3a10f22602a7d2c Mon Sep 17 00:00:00 2001 From: "Simeon H.K. Fitch" Date: Tue, 20 Sep 2022 13:12:34 -0400 Subject: [PATCH 2/5] Changelog. --- CHANGES.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGES.md b/CHANGES.md index ef35727d2..16bbf7e15 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -21,7 +21,7 @@ - Added quality-of-life features to `CslStringList`: `len`, `is_empty`, `Debug` and `Iterator` implementations. - - <> + - ## 0.13 From 9897d1a136651516369d33d740b9bc8eaeb2957e Mon Sep 17 00:00:00 2001 From: "Simeon H.K. Fitch" Date: Thu, 22 Sep 2022 09:56:54 -0400 Subject: [PATCH 3/5] PR feedback. --- src/cpl.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/cpl.rs b/src/cpl.rs index df0353e9f..64ad032a2 100644 --- a/src/cpl.rs +++ b/src/cpl.rs @@ -73,7 +73,7 @@ impl CslStringList { /// Determine the number of entries in the list. pub fn len(&self) -> isize { - unsafe { CSLCount(self.as_ptr()) as isize } + (unsafe { CSLCount(self.as_ptr()) }) as isize } /// Determine if the list has any values @@ -86,7 +86,7 @@ impl CslStringList { CslStringListIterator::new(self) } - /// Get the raw pointer behind list's data. + /// Get the raw pointer to the underlying data. pub fn as_ptr(&self) -> gdal_sys::CSLConstList { self.list_ptr } From f68bd15bc69a23068fe1639493129833fbab0421 Mon Sep 17 00:00:00 2001 From: "Simeon H.K. Fitch" Date: Thu, 22 Sep 2022 12:39:33 -0400 Subject: [PATCH 4/5] Fixed new clippy error. --- src/dataset.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/dataset.rs b/src/dataset.rs index af0ea97b5..ce1f9cc40 100644 --- a/src/dataset.rs +++ b/src/dataset.rs @@ -149,7 +149,7 @@ impl GeoTransformEx for GeoTransform { let rv = unsafe { gdal_sys::GDALInvGeoTransform( self.as_ptr() as *mut f64, - (&mut *gt_out.as_mut_ptr()).as_mut_ptr(), + (*gt_out.as_mut_ptr()).as_mut_ptr(), ) }; if rv == 0 { From b2d63998f403360f40a0d1ab62695db41cd4723b Mon Sep 17 00:00:00 2001 From: "Simeon H.K. Fitch" Date: Thu, 22 Sep 2022 12:54:15 -0400 Subject: [PATCH 5/5] Alternative, more efficient iteration. --- src/cpl.rs | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/src/cpl.rs b/src/cpl.rs index 64ad032a2..bfaa5efd1 100644 --- a/src/cpl.rs +++ b/src/cpl.rs @@ -7,7 +7,7 @@ use std::ffi::CString; use std::fmt::{Debug, Formatter}; use std::ptr; -use gdal_sys::{CSLCount, CSLDestroy, CSLFetchNameValue, CSLGetField, CSLSetNameValue}; +use gdal_sys::{CSLCount, CSLDestroy, CSLFetchNameValue, CSLSetNameValue}; use libc::c_char; use crate::errors::{GdalError, Result}; @@ -72,13 +72,13 @@ impl CslStringList { } /// Determine the number of entries in the list. - pub fn len(&self) -> isize { - (unsafe { CSLCount(self.as_ptr()) }) as isize + pub fn len(&self) -> usize { + (unsafe { CSLCount(self.as_ptr()) }) as usize } /// Determine if the list has any values pub fn is_empty(&self) -> bool { - self.len() <= 0 + self.len() == 0 } /// Get an iterator over the name/value elements of the list. @@ -106,8 +106,8 @@ impl Default for CslStringList { pub struct CslStringListIterator<'a> { list: &'a CslStringList, - idx: isize, - count: isize, + idx: usize, + count: usize, } impl<'a> CslStringListIterator<'a> { @@ -131,7 +131,12 @@ impl<'a> Iterator for CslStringListIterator<'a> { return None; } - let field = unsafe { CSLGetField(self.list.as_ptr(), self.idx as libc::c_int) }; + let field = unsafe { + // Equivalent to, but less traversals than: + // CSLGetField(self.list.as_ptr(), self.idx as libc::c_int) + let slice = std::slice::from_raw_parts(self.list.list_ptr, self.count); + slice[self.idx] + }; if field.is_null() { None } else {