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

Fixes leakage of GDALDataType::Type in RasterBand. #334

Merged
merged 7 commits into from
Nov 29, 2022

Conversation

metasim
Copy link
Contributor

@metasim metasim commented Nov 13, 2022

  • 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.

Also fixes related rustdoc, and cleans up gdal_sys test warnings.

Closes #333

src/driver.rs Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
@metasim metasim added the bug label Nov 19, 2022
* master:
  Hold back geo-types version; deprecations addressed in georust#339
  Fixes deprecations introduced in geo-types v0.7.8.
  Hold back chrono version, as it's addressed in georust#336
  Update .cargo/config.toml
  Fix deprecations from chrono 0.4.23.
gdal-sys/src/lib.rs Outdated Show resolved Hide resolved
unsafe { gdal_sys::GDALGetRasterDataType(self.c_rasterband) }
pub fn band_type(&self) -> GdalDataType {
let ordinal = unsafe { gdal_sys::GDALGetRasterDataType(self.c_rasterband) };
ordinal.try_into().unwrap_or(GdalDataType::Unknown)
Copy link
Member

Choose a reason for hiding this comment

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

Just looking at the code in the browser, can we call GdalType::datatype() here instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lnicola Not sure what you mean... GdalType::datatype() would be to get the datatype most closely associated with a static primitive. In this case, we're getting a c_int from C-GDAL and need to map that into the GdalDataType enum. But perhaps I'm missing what you're suggesting?

Copy link
Member

Choose a reason for hiding this comment

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

Basically ordinal.datatype() instead of ordinal.try_into().unwrap_or(GdalDataType::Unknown). Maybe it doesn't work for c_int, though?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeh, it doesn't work. GdalType is a (wierd) type-level trait, with no self parameter:

fn datatype() -> GdalDataType {

In Scala I'd have called this an "evidence" trait. So <u8>::datatype() exists, but 3u8.datatype() does not.

CHANGES.md Outdated Show resolved Hide resolved
metasim and others added 2 commits November 28, 2022 13:48
Co-authored-by: Laurențiu Nicola <lnicola@users.noreply.github.com>
@metasim metasim requested a review from lnicola November 28, 2022 18:52
@lnicola
Copy link
Member

lnicola commented Nov 29, 2022

@bors d+

@metasim
Copy link
Contributor Author

metasim commented Nov 29, 2022

bors r+

@bors bors bot merged commit 5f6591d into georust:master Nov 29, 2022
@metasim metasim deleted the fix/333 branch November 29, 2022 20:37
@metasim metasim mentioned this pull request May 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

gdal::raster::rasterband::RasterBand::band_type still returns a GDALDataType::Type
3 participants