From 1d21718253484f1a53f408c5b2efb4dd7e8cdb83 Mon Sep 17 00:00:00 2001 From: "Simeon H.K. Fitch" Date: Sat, 21 Oct 2023 17:09:45 -0400 Subject: [PATCH 1/6] Reworked `CslStringListIterator` and added `CslStringList::get_field`, `CslStringList::find_string`, `CslStringList::partial_find_string`, `CslStringList::find_string_case_sensitive` --- CHANGES.md | 6 ++ src/cpl.rs | 221 +++++++++++++++++++++++++++++++++------- src/raster/rasterize.rs | 10 +- 3 files changed, 195 insertions(+), 42 deletions(-) diff --git a/CHANGES.md b/CHANGES.md index 70c30100d..d5bd54c8c 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -2,6 +2,12 @@ ## Unreleased + - **Breaking**: `CslStringListIterator` returns a `CslStringListEntry` instead of `(String, String)` in order to differentiate between `key=value` entries vs `flag` entries. + - **Breaking**: `CslStringList::fetch_name_value` returns `Option` instead of `Result>`, better reflecting the semantics of GDAL C API. + - Added `CslStringList::get_field`, `CslStringList::find_string`, `CslStringList::partial_find_string`, `CslStringList::find_string_case_sensitive` + + - <> + - **Breaking**: `ExtendedDataType` no longer implements `Clone`, `PartialEq` and `Eq` - diff --git a/src/cpl.rs b/src/cpl.rs index 0fa779059..6ea53107a 100644 --- a/src/cpl.rs +++ b/src/cpl.rs @@ -4,16 +4,15 @@ //! use std::ffi::CString; -use std::fmt::{Debug, Formatter}; +use std::fmt::{Debug, Display, Formatter}; +use std::ops::Deref; use std::ptr; -use gdal_sys::{ - CSLAddString, CSLCount, CSLDestroy, CSLDuplicate, CSLFetchNameValue, CSLSetNameValue, -}; -use libc::c_char; +use gdal_sys::{CSLAddString, CSLCount, CSLDestroy, CSLDuplicate, CSLFetchNameValue, CSLFindString, CSLFindStringCaseSensitive, CSLGetField, CSLPartialFindString, CSLSetNameValue}; +use libc::{c_char, c_int}; use crate::errors::{GdalError, Result}; -use crate::utils::{_string, _string_tuple}; +use crate::utils::_string; /// Wraps a [`gdal_sys::CSLConstList`] (a.k.a. `char **papszStrList`). This data structure /// (a null-terminated array of null-terminated strings) is used throughout GDAL to pass @@ -59,9 +58,10 @@ impl CslStringList { Ok(()) } - /// Adds the string `value` to the list. + /// Adds a copy of the string slice `value` to the list. /// - /// Returns `Ok<()>` on success, `Err` if `value` cannot be converted to a C string. + /// Returns `Ok<()>` on success, `Err` if `value` cannot be converted to a C string, + /// e.g. `value` contains a `0` byte, which is used as a string termination sentinel in C. /// /// See: [`CSLAddString`](https://gdal.org/api/cpl.html#_CPPv412CSLAddStringPPcPKc) pub fn add_string(&mut self, value: &str) -> Result<()> { @@ -74,18 +74,90 @@ impl CslStringList { /// /// 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)?; + pub fn fetch_name_value(&self, key: &str) -> Option { + // If CString conversion fails because `key` has an embedded null byte, then + // we know already `key` will never exist in a valid CslStringList. + let key = CString::new(key).ok()?; let c_value = unsafe { CSLFetchNameValue(self.as_ptr(), key.as_ptr()) }; - let value = if c_value.is_null() { + if c_value.is_null() { None } else { Some(_string(c_value)) - }; - Ok(value) + } + } + + /// Perform a case insensitive search for the given string + /// + /// Returns `Some(usize)` of value index position, or `None` if not found. + /// + /// See: [`CSLFindString`](https://gdal.org/api/cpl.html#_CPPv413CSLFindString12CSLConstListPKc) + /// for details. + pub fn find_string(&self, value: &str) -> Option { + let value = CString::new(value).ok()?; + let idx = unsafe { CSLFindString(self.as_ptr(), value.as_ptr()) }; + if idx < 0 { + None + } else { + Some(idx as usize) + } + } + + /// Perform a case sensitive search for the given string + /// + /// Returns `Some(usize)` of value index position, or `None` if not found. + /// + /// See: [`CSLFindString`](https://gdal.org/api/cpl.html#_CPPv413CSLFindString12CSLConstListPKc) + /// for details. + pub fn find_string_case_sensitive(&self, value: &str) -> Option { + let value = CString::new(value).ok()?; + let idx = unsafe { CSLFindStringCaseSensitive(self.as_ptr(), value.as_ptr()) }; + if idx < 0 { + None + } else { + Some(idx as usize) + } + } + + /// Perform a case sensitive partial string search indicated by `fragment`. + /// + /// Returns `Some(usize)` of value index position, or `None` if not found. + /// + /// See:: [`CSLPartialFindString`](https://gdal.org/api/cpl.html#_CPPv420CSLPartialFindString12CSLConstListPKc) + /// for details. + pub fn partial_find_string(&self, fragment: &str) -> Option { + let fragment = CString::new(fragment).ok()?; + let idx = unsafe { CSLPartialFindString(self.as_ptr(), fragment.as_ptr()) }; + if idx < 0 { + None + } else { + Some(idx as usize) + } + } + + /// Fetch the [CslStringListEntry] for the entry at the given index. + /// + /// Returns `None` if index is out of bounds + pub fn get_field(&self, index: usize) -> Option { + // 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()) + } } /// Determine the number of entries in the list. + /// + /// See [`CSLCount`](https://gdal.org/api/cpl.html#_CPPv48CSLCount12CSLConstList) for details. pub fn len(&self) -> usize { (unsafe { CSLCount(self.as_ptr()) }) as usize } @@ -95,8 +167,8 @@ impl CslStringList { self.len() == 0 } - /// Get an iterator over the name/value elements of the list. - pub fn iter(&self) -> CslStringListIterator { + /// Get an iterator over the `name=value` elements of the list. + pub fn iter<'a>(&'a self) -> impl Iterator + 'a { CslStringListIterator::new(self) } @@ -125,7 +197,55 @@ impl Clone for CslStringList { } } +impl Debug for CslStringList { + fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { + let mut b = f.debug_tuple("CslStringList"); + + for e in self.iter() { + b.field(&e.to_string()); + } + + b.finish() + } +} + +/// Represents an entry in a [CslStringList], which is ether a single token, or a key/value assignment. +#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)] +pub enum CslStringListEntry { + Arg(String), + Assign { key: String, value: String }, +} + +impl From<&str> for CslStringListEntry { + fn from(value: &str) -> Self { + match value.split_once('=') { + Some(kv) => kv.into(), + None => Self::Arg(value.to_owned()), + } + } +} + +impl From<(&str, &str)> for CslStringListEntry { + fn from((key, value): (&str, &str)) -> Self { + Self::Assign { + key: key.to_owned(), + value: value.to_owned(), + } + } +} + +impl Display for CslStringListEntry { + fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { + match self { + CslStringListEntry::Arg(s) => f.write_str(s), + CslStringListEntry::Assign { key, value } => f.write_fmt(format_args!("{key}={value}")), + } + } +} + /// State for iterator over [`CslStringList`] entries. +/// +/// Note: Does not include values inserted with [CslStringList::add_string] pub struct CslStringListIterator<'a> { list: &'a CslStringList, idx: usize, @@ -146,7 +266,7 @@ impl<'a> CslStringListIterator<'a> { } impl<'a> Iterator for CslStringListIterator<'a> { - type Item = (String, String); + type Item = CslStringListEntry; fn next(&mut self) -> Option { if self.is_done() { @@ -159,21 +279,13 @@ impl<'a> Iterator for CslStringListIterator<'a> { let slice = std::slice::from_raw_parts(self.list.list_ptr, self.count); slice[self.idx] }; + self.idx += 1; 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"))?; + let entry = _string(field); + Some(entry.deref().into()) } - Ok(()) } } @@ -209,16 +321,16 @@ mod tests { l.set_name_value("ONE", "1")?; l.set_name_value("TWO", "2")?; l.set_name_value("THREE", "3")?; - + l.add_string("SOME_FLAG")?; 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))); + assert!(matches!(l.fetch_name_value("ONE"), Some(s) if s == *"1")); + assert!(matches!(l.fetch_name_value("THREE"), Some(s) if s == *"3")); + assert!(matches!(l.fetch_name_value("FOO"), None)); Ok(()) } @@ -226,7 +338,7 @@ mod tests { #[test] fn has_length() -> Result<()> { let l = fixture()?; - assert_eq!(l.len(), 3); + assert_eq!(l.len(), 4); Ok(()) } @@ -246,9 +358,10 @@ mod tests { 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(), Some(("ONE", "1").into())); + assert_eq!(it.next(), Some(("TWO", "2").into())); + assert_eq!(it.next(), Some(("THREE", "3").into())); + assert_eq!(it.next(), Some("SOME_FLAG".into())); assert_eq!(it.next(), None); assert_eq!(it.next(), None); Ok(()) @@ -266,8 +379,8 @@ mod tests { #[test] fn try_from_impl() -> Result<()> { let l = CslStringList::try_from(&[("ONE", "1"), ("TWO", "2")])?; - assert!(matches!(l.fetch_name_value("ONE"), Ok(Some(s)) if s == *"1")); - assert!(matches!(l.fetch_name_value("TWO"), Ok(Some(s)) if s == *"2")); + assert!(matches!(l.fetch_name_value("ONE"), Some(s) if s == *"1")); + assert!(matches!(l.fetch_name_value("TWO"), Some(s) if s == *"2")); Ok(()) } @@ -279,6 +392,7 @@ mod tests { assert!(s.contains("ONE=1")); assert!(s.contains("TWO=2")); assert!(s.contains("THREE=3")); + assert!(s.contains("SOME_FLAG")); Ok(()) } @@ -295,4 +409,37 @@ mod tests { Ok(()) } + + #[test] + fn find_string() -> Result<()> { + let f = fixture()?; + assert_eq!(f.find_string("NON_FLAG"), None); + assert_eq!(f.find_string("SOME_FLAG"), Some(3)); + assert_eq!(f.find_string("ONE=1"), Some(0)); + assert_eq!(f.find_string("one=1"), Some(0)); + assert_eq!(f.find_string("TWO="), None); + Ok(()) + } + + #[test] + fn find_string_case_sensitive() -> Result<()> { + let f = fixture()?; + assert_eq!(f.find_string_case_sensitive("ONE=1"), Some(0)); + assert_eq!(f.find_string_case_sensitive("one=1"), None); + assert_eq!(f.find_string_case_sensitive("SOME_FLAG"), Some(3)); + Ok(()) + } + + #[test] + fn partial_find_string() -> Result<()> { + let f = fixture()?; + assert_eq!(f.partial_find_string("ONE=1"), Some(0)); + assert_eq!(f.partial_find_string("ONE="), Some(0)); + assert_eq!(f.partial_find_string("=1"), Some(0)); + assert_eq!(f.partial_find_string("1"), Some(0)); + assert_eq!(f.partial_find_string("THREE="), Some(2)); + assert_eq!(f.partial_find_string("THREE"), Some(2)); + assert_eq!(f.partial_find_string("three"), None); + Ok(()) + } } diff --git a/src/raster/rasterize.rs b/src/raster/rasterize.rs index baf3d2b8d..7b244eb87 100644 --- a/src/raster/rasterize.rs +++ b/src/raster/rasterize.rs @@ -128,20 +128,20 @@ mod tests { fn test_rasterizeoptions_as_ptr() { let c_options = CslStringList::try_from(RasterizeOptions::default()).unwrap(); assert_eq!( - c_options.fetch_name_value("ALL_TOUCHED").unwrap(), + c_options.fetch_name_value("ALL_TOUCHED"), Some("FALSE".to_string()) ); - assert_eq!(c_options.fetch_name_value("BURN_VALUE_FROM").unwrap(), None); + assert_eq!(c_options.fetch_name_value("BURN_VALUE_FROM"), None); assert_eq!( - c_options.fetch_name_value("MERGE_ALG").unwrap(), + c_options.fetch_name_value("MERGE_ALG"), Some("REPLACE".to_string()) ); assert_eq!( - c_options.fetch_name_value("CHUNKYSIZE").unwrap(), + c_options.fetch_name_value("CHUNKYSIZE"), Some("0".to_string()) ); assert_eq!( - c_options.fetch_name_value("OPTIM").unwrap(), + c_options.fetch_name_value("OPTIM"), Some("AUTO".to_string()) ); } From 6b1a934678f61243489de45ebc3b1db0412ea9e6 Mon Sep 17 00:00:00 2001 From: "Simeon H.K. Fitch" Date: Mon, 23 Oct 2023 15:25:35 -0400 Subject: [PATCH 2/6] Added `CslStringList::into_ptr` Added documentation. --- CHANGES.md | 2 +- src/cpl.rs | 155 +++++++++++++++++++++++++++++++++++++++++------------ 2 files changed, 123 insertions(+), 34 deletions(-) diff --git a/CHANGES.md b/CHANGES.md index d5bd54c8c..5c5c6f4ba 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -4,7 +4,7 @@ - **Breaking**: `CslStringListIterator` returns a `CslStringListEntry` instead of `(String, String)` in order to differentiate between `key=value` entries vs `flag` entries. - **Breaking**: `CslStringList::fetch_name_value` returns `Option` instead of `Result>`, better reflecting the semantics of GDAL C API. - - Added `CslStringList::get_field`, `CslStringList::find_string`, `CslStringList::partial_find_string`, `CslStringList::find_string_case_sensitive` + - Added `CslStringList::get_field`, `CslStringList::find_string`, `CslStringList::partial_find_string`, `CslStringList::find_string_case_sensitive`, `CslStringList::into_ptr` - <> diff --git a/src/cpl.rs b/src/cpl.rs index 6ea53107a..4a8bcf47c 100644 --- a/src/cpl.rs +++ b/src/cpl.rs @@ -5,18 +5,43 @@ use std::ffi::CString; use std::fmt::{Debug, Display, Formatter}; +use std::mem::ManuallyDrop; use std::ops::Deref; use std::ptr; +use std::str::FromStr; -use gdal_sys::{CSLAddString, CSLCount, CSLDestroy, CSLDuplicate, CSLFetchNameValue, CSLFindString, CSLFindStringCaseSensitive, CSLGetField, CSLPartialFindString, CSLSetNameValue}; +use gdal_sys::{ + CSLAddString, CSLCount, CSLDestroy, CSLDuplicate, CSLFetchNameValue, CSLFindString, + CSLFindStringCaseSensitive, CSLGetField, CSLPartialFindString, CSLSetNameValue, + CSLTokenizeString2, +}; use libc::{c_char, c_int}; use crate::errors::{GdalError, Result}; use crate::utils::_string; -/// Wraps a [`gdal_sys::CSLConstList`] (a.k.a. `char **papszStrList`). This data structure -/// (a null-terminated array of null-terminated strings) is used throughout GDAL to pass -/// `KEY=VALUE`-formatted options to various functions. +/// Wraps a [`gdal_sys::CSLConstList`] (a.k.a. `char **papszStrList`). +/// +/// This data structure (a null-terminated array of null-terminated strings) is used throughout +/// GDAL to pass `KEY=VALUE`-formatted options to various functions. +/// +/// There are a number of ways to populate a [`CslStringList`]. See below for examples. +/// +/// # Example +/// +/// ```rust +/// use gdal::cpl::CslStringList; +/// +/// let mut sl1 = CslStringList::new(); +/// sl1.set_name_value("NUM_THREADS", "ALL_CPUS").unwrap(); +/// sl1.set_name_value("COMPRESS", "LZW").unwrap(); +/// +/// let sl2: CslStringList = "NUM_THREADS=ALL_CPUS COMPRESS=LZW".parse().unwrap(); +/// let sl3: CslStringList = (&[("NUM_THREADS", "ALL_CPUS"), ("COMPRESS", "LZW")]).try_into().unwrap(); +/// +/// assert_eq!(sl1, sl2); +/// assert_eq!(sl2, sl3); +/// ``` /// /// See the [`CSL*` GDAL functions](https://gdal.org/api/cpl.html#cpl-string-h) for more details. pub struct CslStringList { @@ -122,7 +147,7 @@ impl CslStringList { /// /// Returns `Some(usize)` of value index position, or `None` if not found. /// - /// See:: [`CSLPartialFindString`](https://gdal.org/api/cpl.html#_CPPv420CSLPartialFindString12CSLConstListPKc) + /// See: [`CSLPartialFindString`](https://gdal.org/api/cpl.html#_CPPv420CSLPartialFindString12CSLConstListPKc) /// for details. pub fn partial_find_string(&self, fragment: &str) -> Option { let fragment = CString::new(fragment).ok()?; @@ -143,21 +168,20 @@ impl CslStringList { // 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 + return None; } let field = _string(field); if field.is_empty() { None - } - else { + } else { Some(field.deref().into()) } } /// Determine the number of entries in the list. /// - /// See [`CSLCount`](https://gdal.org/api/cpl.html#_CPPv48CSLCount12CSLConstList) for details. + /// See: [`CSLCount`](https://gdal.org/api/cpl.html#_CPPv48CSLCount12CSLConstList) for details. pub fn len(&self) -> usize { (unsafe { CSLCount(self.as_ptr()) }) as usize } @@ -168,7 +192,7 @@ impl CslStringList { } /// Get an iterator over the `name=value` elements of the list. - pub fn iter<'a>(&'a self) -> impl Iterator + 'a { + pub fn iter(&self) -> impl Iterator + '_ { CslStringListIterator::new(self) } @@ -176,6 +200,13 @@ impl CslStringList { pub fn as_ptr(&self) -> gdal_sys::CSLConstList { self.list_ptr } + + /// Get the raw pointer to the underlying data, passing ownership + /// (and responsibility for freeing) to the the caller. + pub fn into_ptr(self) -> gdal_sys::CSLConstList { + let s = ManuallyDrop::new(self); + s.list_ptr + } } impl Drop for CslStringList { @@ -209,6 +240,72 @@ impl Debug for CslStringList { } } +impl Display for CslStringList { + fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { + // CSLPrint would be preferable here, but it can only write to a file descriptor. + let len = self.len(); + for i in 0..len { + if let Some(field) = self.get_field(i) { + f.write_fmt(format_args!("{field}\n"))?; + } + } + Ok(()) + } +} + +impl PartialEq for CslStringList { + fn eq(&self, other: &Self) -> bool { + // Not the best comparison mechanism, but C API doesn't help us here. + self.to_string() == other.to_string() + } +} + +/// Parse a space-delimited string into a [`CslStringList`]. +/// +/// See [`CSLTokenizeString`](https://gdal.org/api/cpl.html#_CPPv417CSLTokenizeStringPKc) for details +impl FromStr for CslStringList { + type Err = GdalError; + + fn from_str(s: &str) -> std::result::Result { + // See: https://github.com/OSGeo/gdal/blob/cd2a054b0d7b881534baece69a8f52ddb69a53d9/port/cpl_string.h#L86C1-L97 + static CSLT_HONOURSTRINGS: c_int = 0x0001; + static CSLT_PRESERVEESCAPES: c_int = 0x0008; + static DELIM: &[u8; 4] = b" \n\t\0"; + + let cstr = CString::new(s)?; + let c_list = unsafe { + CSLTokenizeString2( + cstr.as_ptr(), + DELIM.as_ptr() as *const c_char, + CSLT_HONOURSTRINGS | CSLT_PRESERVEESCAPES, + ) + }; + Ok(Self { list_ptr: c_list }) + } +} + +/// Convenience for creating a [`CslStringList`] from a slice of _key_/_value_ tuples. +/// +/// # Example +/// +/// ```rust, no_run +/// use gdal::cpl::CslStringList; +/// +/// let opts = CslStringList::try_from(&[("One", "1"), ("Two", "2"), ("Three", "3")]).expect("known valid"); +/// assert_eq!(opts.len(), 3); +/// ``` +impl TryFrom<&[(&str, &str); N]> for CslStringList { + type Error = GdalError; + + fn try_from(pairs: &[(&str, &str); N]) -> Result { + let mut result = Self::default(); + for (k, v) in pairs { + result.set_name_value(k, v)?; + } + Ok(result) + } +} + /// Represents an entry in a [CslStringList], which is ether a single token, or a key/value assignment. #[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)] pub enum CslStringListEntry { @@ -289,28 +386,6 @@ impl<'a> Iterator for CslStringListIterator<'a> { } } -/// Convenience for creating a [`CslStringList`] from a slice of _key_/_value_ tuples. -/// -/// # Example -/// -/// ```rust, no_run -/// use gdal::cpl::CslStringList; -/// -/// let opts = CslStringList::try_from(&[("One", "1"), ("Two", "2"), ("Three", "3")]).expect("known valid"); -/// assert_eq!(opts.len(), 3); -/// ``` -impl TryFrom<&[(&str, &str); N]> for CslStringList { - type Error = GdalError; - - fn try_from(pairs: &[(&str, &str); N]) -> Result { - let mut result = Self::default(); - for (k, v) in pairs { - result.set_name_value(k, v)?; - } - Ok(result) - } -} - #[cfg(test)] mod tests { use crate::cpl::CslStringList; @@ -330,7 +405,7 @@ mod tests { let l = fixture()?; assert!(matches!(l.fetch_name_value("ONE"), Some(s) if s == *"1")); assert!(matches!(l.fetch_name_value("THREE"), Some(s) if s == *"3")); - assert!(matches!(l.fetch_name_value("FOO"), None)); + assert!(l.fetch_name_value("FOO").is_none()); Ok(()) } @@ -442,4 +517,18 @@ mod tests { assert_eq!(f.partial_find_string("three"), None); Ok(()) } + + #[test] + fn parse() -> Result<()> { + let f = fixture()?; + let s = f.to_string(); + let r: CslStringList = s.parse()?; + + assert_eq!(f.len(), r.len()); + assert_eq!(r.find_string("SOME_FLAG"), Some(3)); + assert_eq!(f.partial_find_string("THREE="), Some(2)); + assert_eq!(s, r.to_string()); + + Ok(()) + } } From ccfc093aa7acb9efcbe2544d0ae65eddbcbf00ed Mon Sep 17 00:00:00 2001 From: "Simeon H.K. Fitch" Date: Mon, 23 Oct 2023 15:34:38 -0400 Subject: [PATCH 3/6] Changelog link. [skip ci] --- CHANGES.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGES.md b/CHANGES.md index 5c5c6f4ba..b4f81689f 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -6,7 +6,7 @@ - **Breaking**: `CslStringList::fetch_name_value` returns `Option` instead of `Result>`, better reflecting the semantics of GDAL C API. - Added `CslStringList::get_field`, `CslStringList::find_string`, `CslStringList::partial_find_string`, `CslStringList::find_string_case_sensitive`, `CslStringList::into_ptr` - - <> + - - **Breaking**: `ExtendedDataType` no longer implements `Clone`, `PartialEq` and `Eq` From c8ad77e2df3b603646379ae0fc88d47e1ba6b8b3 Mon Sep 17 00:00:00 2001 From: "Simeon H.K. Fitch" Date: Tue, 24 Oct 2023 14:26:52 -0400 Subject: [PATCH 4/6] Apply suggestions from code review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Laurențiu Nicola --- src/cpl.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/cpl.rs b/src/cpl.rs index 4a8bcf47c..487d1ad7e 100644 --- a/src/cpl.rs +++ b/src/cpl.rs @@ -159,7 +159,7 @@ impl CslStringList { } } - /// Fetch the [CslStringListEntry] for the entry at the given index. + /// Fetch the [`CslStringListEntry`] for the entry at the given index. /// /// Returns `None` if index is out of bounds pub fn get_field(&self, index: usize) -> Option { @@ -306,7 +306,7 @@ impl TryFrom<&[(&str, &str); N]> for CslStringList { } } -/// Represents an entry in a [CslStringList], which is ether a single token, or a key/value assignment. +/// Represents an entry in a [`CslStringList`], which is ether a single token, or a key/value assignment. #[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)] pub enum CslStringListEntry { Arg(String), @@ -342,7 +342,7 @@ impl Display for CslStringListEntry { /// State for iterator over [`CslStringList`] entries. /// -/// Note: Does not include values inserted with [CslStringList::add_string] +/// Note: Does not include values inserted with [`CslStringList::add_string`] pub struct CslStringListIterator<'a> { list: &'a CslStringList, idx: usize, From c1c7fe0950f51a81aea6cc66a437a52325236024 Mon Sep 17 00:00:00 2001 From: "Simeon H.K. Fitch" Date: Tue, 24 Oct 2023 14:50:26 -0400 Subject: [PATCH 5/6] Binding for `CSLAddNameValue`. --- CHANGES.md | 2 +- src/cpl.rs | 92 +++++++++++++++++++++++++++++++++++++++++++++--------- 2 files changed, 78 insertions(+), 16 deletions(-) diff --git a/CHANGES.md b/CHANGES.md index b4f81689f..8c9a1e4c8 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -4,7 +4,7 @@ - **Breaking**: `CslStringListIterator` returns a `CslStringListEntry` instead of `(String, String)` in order to differentiate between `key=value` entries vs `flag` entries. - **Breaking**: `CslStringList::fetch_name_value` returns `Option` instead of `Result>`, better reflecting the semantics of GDAL C API. - - Added `CslStringList::get_field`, `CslStringList::find_string`, `CslStringList::partial_find_string`, `CslStringList::find_string_case_sensitive`, `CslStringList::into_ptr` + - Added `CslStringList::get_field`, `CslStringList::find_string`, `CslStringList::partial_find_string`, `CslStringList::find_string_case_sensitive`, `CslStringList::into_ptr`, `CslStringList::add_name_value`. - diff --git a/src/cpl.rs b/src/cpl.rs index 487d1ad7e..19d34bc89 100644 --- a/src/cpl.rs +++ b/src/cpl.rs @@ -10,11 +10,7 @@ use std::ops::Deref; use std::ptr; use std::str::FromStr; -use gdal_sys::{ - CSLAddString, CSLCount, CSLDestroy, CSLDuplicate, CSLFetchNameValue, CSLFindString, - CSLFindStringCaseSensitive, CSLGetField, CSLPartialFindString, CSLSetNameValue, - CSLTokenizeString2, -}; +use gdal_sys::{CSLAddNameValue, CSLAddString, CSLCount, CSLDestroy, CSLDuplicate, CSLFetchNameValue, CSLFindString, CSLFindStringCaseSensitive, CSLGetField, CSLPartialFindString, CSLSetNameValue, CSLTokenizeString2}; use libc::{c_char, c_int}; use crate::errors::{GdalError, Result}; @@ -56,23 +52,72 @@ impl CslStringList { } } - /// Assigns `value` to `name`. + /// Check that the given `name` is a valid [`CslStringList`] key. /// - /// Overwrites duplicate `name`s. + /// Per [GDAL documentation](https://gdal.org/api/cpl.html#_CPPv415CSLSetNameValuePPcPKcPKc), + /// a key cannot have non-alphanumeric characters in it. /// - /// 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<()> { + /// Returns `Err(GdalError::BadArgument)` on invalid name, `Ok(())` otherwise. + fn check_valid_name(name: &str) -> Result<()> { if !name.chars().all(|c| c.is_ascii_alphanumeric() || c == '_') { - return Err(GdalError::BadArgument(format!( + Err(GdalError::BadArgument(format!( "Invalid characters in name: '{name}'" - ))); + ))) + } + else { + Ok(()) } + } + + /// Check that the given `value` is a valid [`CslStringList`] value. + /// + /// Per [GDAL documentation](https://gdal.org/api/cpl.html#_CPPv415CSLSetNameValuePPcPKcPKc), + /// a key cannot have newline characters in it. + /// + /// Returns `Err(GdalError::BadArgument)` on invalid value, `Ok(())` otherwise. + fn check_valid_value(value: &str) -> Result<()> { if value.contains(|c| c == '\n' || c == '\r') { - return Err(GdalError::BadArgument(format!( + Err(GdalError::BadArgument(format!( "Invalid characters in value: '{value}'" - ))); + ))) + } + else { + Ok(()) } + } + + /// Assigns `value` to the key `name` without checking for a pre-existing assignments. + /// + /// Returns `Ok<()>` on success, or `Err` + /// if `name` has non-alphanumeric characters or `value` has newline characters. + /// + /// See: [`CSLAddNameValue`](https://gdal.org/api/cpl.html#_CPPv415CSLAddNameValuePPcPKcPKc) + /// for details. + pub fn add_name_value(&mut self, name: &str, value: &str) -> Result<()> { + Self::check_valid_name(name)?; + Self::check_valid_value(value)?; + + let psz_name = CString::new(name)?; + let psz_value = CString::new(value)?; + + unsafe { + self.list_ptr = CSLAddNameValue(self.list_ptr, psz_name.as_ptr(), psz_value.as_ptr()); + } + + Ok(()) + } + + /// Assigns `value` to the key `name`, overwriting any existing assignment to `name`. + /// + /// Returns `Ok<()>` on success, or `Err` + /// if `name` has non-alphanumeric characters or `value` has newline characters. + /// + /// See: [`CSLSetNameValue`](https://gdal.org/api/cpl.html#_CPPv415CSLSetNameValuePPcPKcPKc) + /// for details. + pub fn set_name_value(&mut self, name: &str, value: &str) -> Result<()> { + Self::check_valid_name(name)?; + Self::check_valid_value(value)?; + let psz_name = CString::new(name)?; let psz_value = CString::new(value)?; @@ -443,7 +488,7 @@ mod tests { } #[test] - fn invalid_keys() -> Result<()> { + fn invalid_name_value() -> 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()); @@ -451,6 +496,23 @@ mod tests { Ok(()) } + #[test] + fn add_vs_set() -> Result<()> { + let mut f = CslStringList::new(); + f.add_name_value("ONE", "1")?; + f.add_name_value("ONE", "2")?; + let s = f.to_string(); + assert!(s.contains("ONE") && s.contains("1") && s.contains("2")); + + let mut f = CslStringList::new(); + f.set_name_value("ONE", "1")?; + f.set_name_value("ONE", "2")?; + let s = f.to_string(); + assert!(s.contains("ONE") && !s.contains("1") && s.contains("2")); + + Ok(()) + } + #[test] fn try_from_impl() -> Result<()> { let l = CslStringList::try_from(&[("ONE", "1"), ("TWO", "2")])?; From 6e50c4db25d5e6f6618caf48d37ea503c0a88794 Mon Sep 17 00:00:00 2001 From: "Simeon H.K. Fitch" Date: Tue, 24 Oct 2023 15:04:52 -0400 Subject: [PATCH 6/6] Misc PR feedback. --- src/cpl.rs | 76 +++++++++++++++++++++++++++--------------------------- 1 file changed, 38 insertions(+), 38 deletions(-) diff --git a/src/cpl.rs b/src/cpl.rs index 19d34bc89..a9a86bd1b 100644 --- a/src/cpl.rs +++ b/src/cpl.rs @@ -10,7 +10,11 @@ use std::ops::Deref; use std::ptr; use std::str::FromStr; -use gdal_sys::{CSLAddNameValue, CSLAddString, CSLCount, CSLDestroy, CSLDuplicate, CSLFetchNameValue, CSLFindString, CSLFindStringCaseSensitive, CSLGetField, CSLPartialFindString, CSLSetNameValue, CSLTokenizeString2}; +use gdal_sys::{ + CSLAddNameValue, CSLAddString, CSLCount, CSLDestroy, CSLDuplicate, CSLFetchNameValue, + CSLFindString, CSLFindStringCaseSensitive, CSLGetField, CSLPartialFindString, CSLSetNameValue, + CSLTokenizeString2, +}; use libc::{c_char, c_int}; use crate::errors::{GdalError, Result}; @@ -35,8 +39,8 @@ use crate::utils::_string; /// let sl2: CslStringList = "NUM_THREADS=ALL_CPUS COMPRESS=LZW".parse().unwrap(); /// let sl3: CslStringList = (&[("NUM_THREADS", "ALL_CPUS"), ("COMPRESS", "LZW")]).try_into().unwrap(); /// -/// assert_eq!(sl1, sl2); -/// assert_eq!(sl2, sl3); +/// assert_eq!(sl1.to_string(), sl2.to_string()); +/// assert_eq!(sl2.to_string(), sl3.to_string()); /// ``` /// /// See the [`CSL*` GDAL functions](https://gdal.org/api/cpl.html#cpl-string-h) for more details. @@ -63,8 +67,7 @@ impl CslStringList { Err(GdalError::BadArgument(format!( "Invalid characters in name: '{name}'" ))) - } - else { + } else { Ok(()) } } @@ -80,15 +83,14 @@ impl CslStringList { Err(GdalError::BadArgument(format!( "Invalid characters in value: '{value}'" ))) - } - else { + } else { Ok(()) } } /// Assigns `value` to the key `name` without checking for a pre-existing assignments. /// - /// Returns `Ok<()>` on success, or `Err` + /// Returns `Ok(())` on success, or `Err(GdalError::BadArgument)` /// if `name` has non-alphanumeric characters or `value` has newline characters. /// /// See: [`CSLAddNameValue`](https://gdal.org/api/cpl.html#_CPPv415CSLAddNameValuePPcPKcPKc) @@ -109,7 +111,7 @@ impl CslStringList { /// Assigns `value` to the key `name`, overwriting any existing assignment to `name`. /// - /// Returns `Ok<()>` on success, or `Err` + /// Returns `Ok(())` on success, or `Err(GdalError::BadArgument)` /// if `name` has non-alphanumeric characters or `value` has newline characters. /// /// See: [`CSLSetNameValue`](https://gdal.org/api/cpl.html#_CPPv415CSLSetNameValuePPcPKcPKc) @@ -130,7 +132,7 @@ impl CslStringList { /// Adds a copy of the string slice `value` to the list. /// - /// Returns `Ok<()>` on success, `Err` if `value` cannot be converted to a C string, + /// Returns `Ok(())` on success, `Err(GdalError::FfiNulError)` if `value` cannot be converted to a C string, /// e.g. `value` contains a `0` byte, which is used as a string termination sentinel in C. /// /// See: [`CSLAddString`](https://gdal.org/api/cpl.html#_CPPv412CSLAddStringPPcPKc) @@ -140,14 +142,14 @@ impl CslStringList { Ok(()) } - /// Looks up the value corresponding to `key`. + /// Looks up the value corresponding to `name`. /// /// See [`CSLFetchNameValue`](https://gdal.org/doxygen/cpl__string_8h.html#a4f23675f8b6f015ed23d9928048361a1) /// for details. - pub fn fetch_name_value(&self, key: &str) -> Option { + pub fn fetch_name_value(&self, name: &str) -> Option { // If CString conversion fails because `key` has an embedded null byte, then - // we know already `key` will never exist in a valid CslStringList. - let key = CString::new(key).ok()?; + // 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 @@ -206,8 +208,11 @@ impl CslStringList { /// Fetch the [`CslStringListEntry`] for the entry at the given index. /// - /// Returns `None` if index is out of bounds + /// Returns `None` if index is out of bounds, `Some(entry)` otherwise. pub fn get_field(&self, index: usize) -> Option { + 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 @@ -237,7 +242,7 @@ impl CslStringList { } /// Get an iterator over the `name=value` elements of the list. - pub fn iter(&self) -> impl Iterator + '_ { + pub fn iter(&self) -> CslStringListIterator { CslStringListIterator::new(self) } @@ -288,23 +293,13 @@ impl Debug for CslStringList { impl Display for CslStringList { fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { // CSLPrint would be preferable here, but it can only write to a file descriptor. - let len = self.len(); - for i in 0..len { - if let Some(field) = self.get_field(i) { - f.write_fmt(format_args!("{field}\n"))?; - } + for e in self.iter() { + f.write_fmt(format_args!("{e}\n"))?; } Ok(()) } } -impl PartialEq for CslStringList { - fn eq(&self, other: &Self) -> bool { - // Not the best comparison mechanism, but C API doesn't help us here. - self.to_string() == other.to_string() - } -} - /// Parse a space-delimited string into a [`CslStringList`]. /// /// See [`CSLTokenizeString`](https://gdal.org/api/cpl.html#_CPPv417CSLTokenizeStringPKc) for details @@ -351,26 +346,29 @@ impl TryFrom<&[(&str, &str); N]> for CslStringList { } } -/// Represents an entry in a [`CslStringList`], which is ether a single token, or a key/value assignment. +/// Represents an entry in a [`CslStringList`], which is ether a single token (`Flag`), +/// or a `name=value` assignment (`Pair`). #[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)] pub enum CslStringListEntry { - Arg(String), - Assign { key: String, value: String }, + /// A single token entry. + Flag(String), + /// A `name=value` pair entry. + Pair { name: String, value: String }, } impl From<&str> for CslStringListEntry { fn from(value: &str) -> Self { match value.split_once('=') { Some(kv) => kv.into(), - None => Self::Arg(value.to_owned()), + None => Self::Flag(value.to_owned()), } } } impl From<(&str, &str)> for CslStringListEntry { fn from((key, value): (&str, &str)) -> Self { - Self::Assign { - key: key.to_owned(), + Self::Pair { + name: key.to_owned(), value: value.to_owned(), } } @@ -379,8 +377,10 @@ impl From<(&str, &str)> for CslStringListEntry { impl Display for CslStringListEntry { fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { match self { - CslStringListEntry::Arg(s) => f.write_str(s), - CslStringListEntry::Assign { key, value } => f.write_fmt(format_args!("{key}={value}")), + CslStringListEntry::Flag(s) => f.write_str(s), + CslStringListEntry::Pair { name: key, value } => { + f.write_fmt(format_args!("{key}={value}")) + } } } } @@ -502,13 +502,13 @@ mod tests { f.add_name_value("ONE", "1")?; f.add_name_value("ONE", "2")?; let s = f.to_string(); - assert!(s.contains("ONE") && s.contains("1") && s.contains("2")); + assert!(s.contains("ONE") && s.contains('1') && s.contains('2')); let mut f = CslStringList::new(); f.set_name_value("ONE", "1")?; f.set_name_value("ONE", "2")?; let s = f.to_string(); - assert!(s.contains("ONE") && !s.contains("1") && s.contains("2")); + assert!(s.contains("ONE") && !s.contains('1') && s.contains('2')); Ok(()) }