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

Made mapping between ResampleAlg and GDALRIOResampleAlg more direct. #309

Merged
merged 3 commits into from Sep 18, 2022

Conversation

metasim
Copy link
Contributor

@metasim metasim commented Sep 9, 2022

Before the private function map_resample_alg was the only way to get the expected GDAL enumeration value.

  • I agree to follow the project's code of conduct.
  • I added an entry to CHANGES.md if knowledge of this change could be valuable to users.

Before this change, you had to copy map_resample_alg (because it was private) when calling into gdal-sys functions needing the resampling algorithm (e.g. GDALAutoCreateWarpedVRT). Instead of making pub fn map_resample_alg ..., took the approach that more tightly binds the C-side types to the Rust enum. Makes the association more clear, and less error prone in the future.

Before the private function `map_resample_alg` was the only way to get
the expected GDAL enumeration value.
}

fn map_resample_alg(alg: &ResampleAlg) -> u32 {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: this was not private before, so there was no easy way to map values when calling into lower level gdal-sys functions.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, the return type should have been GDALRIOResampleAlg::Type.

#[derive(Debug, Copy, Clone)]
#[repr(u32)]
Copy link
Contributor Author

@metasim metasim Sep 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not entirely sure about this value, or even if it should be specified.
GDALRIOResampleAlg::Type is libc::c_uint, which is u32 on *nix. The repr options maybe suggest #[repr(C)] should be used instead, and the right value will be selected based on the discriminant values? Advice appreciated.

Copy link
Member

@lnicola lnicola Sep 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need the #[repr], given the cast in to_gdal. Or does the compiler pick isize there?

EDIT: yeah, I think u32 is fine here. We could also keep the match in to_gdal.

/// assert!(buf.data.iter().all(|c| (c - stats.mean).abs() < stats.std_dev / 2.0));
/// # Ok(())
/// # }
/// ```
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bonus documentation while I was in there :)

@metasim metasim requested review from lnicola and ChristianBeilschmidt and removed request for lnicola September 12, 2022 18:59
@metasim
Copy link
Contributor Author

metasim commented Sep 18, 2022

bors r+

@bors
Copy link
Contributor

bors bot commented Sep 18, 2022

🔒 Permission denied

Existing reviewers: click here to make metasim a reviewer

@lnicola
Copy link
Member

lnicola commented Sep 18, 2022

bors r+

@bors
Copy link
Contributor

bors bot commented Sep 18, 2022

Build succeeded:

@bors bors bot merged commit e52f613 into georust:master Sep 18, 2022
@metasim metasim deleted the feature/resample-alg-id branch September 18, 2022 21:16
@metasim metasim mentioned this pull request Oct 6, 2022
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 this pull request may close these issues.

None yet

2 participants