Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
metasim opened this issue Oct 20, 2023 · 1 comment · Fixed by #455
Closed

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

metasim opened this issue Oct 20, 2023 · 1 comment · Fixed by #455
Assignees

Comments

@metasim
Copy link
Contributor

metasim commented Oct 20, 2023

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
@metasim metasim changed the title CslStringList does not propertly support non-key-value entries. CslStringList does not properly support non-key-value entries. Oct 20, 2023
@lnicola
Copy link
Member

lnicola commented Oct 21, 2023

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.

@metasim metasim self-assigned this Oct 23, 2023
@metasim metasim mentioned this issue Oct 23, 2023
2 tasks
@bors bors bot closed this as completed in 204e470 Oct 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants