georust / gdal

Rust bindings for GDAL
https://crates.io/crates/gdal
MIT License
339 stars 92 forks source link

`CslStringList` does not properly support non-key-value entries. #454

Closed metasim closed 8 months ago

metasim commented 9 months ago

TL;DR:

The design of CslStringList::iter is incomplete, and arguably wrong altogether.

Details

CslStringList Is used extensively in GDAL to pass stringly represented arguments/parameters to a number of GDAL subsystems. It's a glorified wrapper around a C-array of C-strings (*mut *mut c_char). The GDAL API provides a number of convenience methods for manipulating this memory in a slightly more safe way than working with the C pointers directly. The API has grown over time, and has some eccentricities about it that aren't obvious at first glance.

One of these nuances is you can insert two forms of entries:

Name Example Insert Method Fetch Method
key-value "KEY=VALUE" CslStringList::set_name_value CslStringList::fetch_name_value
flag "FLAG" CslStringList::add_string Unimplemented

The current Rust binding design did not fully consider the flag form of entries, at least not from the read/fetch perspective (you can insert them, but never retrieve or view them).

Furthermore, the implementation of CslStringList::iter assumes the key-value form (returning (String, String), and skips over entries of the flag form, as illustrated by this test:

#[test]
fn iter_over_all() -> Result<()>{
    let mut l = CslStringList::new();
    l.set_name_value("ONE", "1")?;
    l.set_name_value("TWO", "2")?;
    l.set_name_value("THREE", "3")?;
    l.add_string("SOME_FLAG")?;

    assert_eq!(l.len(), 4);
    assert_eq!(l.iter().count(), 4); // <-- 'assertion failed: `(left == right)`

    Ok(())
}

As a consequence, Flag form entries are also excluded from Debug formatting, which makes it hard to inspect code having to use CslStringList.

Considerations

In addressing these inadequacies, the following decisions need to be made:

  1. Do we have a single Iterator returning both key-value and flag forms, or do we have two different iterators?
  2. If it's a single Iterator, what's the return type?
  3. To flesh out support for flag form entries, should we also expose the following capabilities?:
    • CSLFindString
    • CSLFindStringCaseSensitive
    • CSLPartialFindString
lnicola commented 9 months ago

One other thing that should be a separate issue, but it's too small: we might want a consuming into_ptr method to prevent double-freeing in cases where GDAL takes ownership of the list. I think I saw this with WarpOptions recently.