Changed a number of APIs using isize when usize is semantically m…

…ore appropriate (georust#497)

* Changed a number of APIs using `isize` when `usize` is semantically more appropriate.

* Use `c_int::try_from`.

* Converted rasterband buffer len/size mismatches from panics to errors.

* Documentation fixes.
[skip ci]

CHANGES.md

@@ -2,6 +2,10 @@
 
 ## Unreleased
 
+- **Breaking**: Changed a number of APIs using `isize` when `usize` is semantically more appropriate: `Driver::create.*`, `Rasterband::overview`, `Dataset::{layer|into_layer|layer_count}`.  
+
+  - <https://github.com/georust/gdal/pull/497>
+
 - Created `enum AxisMappingStrategy` for `OSRAxisMappingStrategy` ordinals.
 - **Breaking**: `SpatialRef::{set_}axis_mapping_strategy` use `AxisMappingStrategy` instead of `gdal_sys::OSRAxisMappingStrategy::Type`.
 

src/driver.rs

@@ -3,7 +3,6 @@ use std::path::Path;
 use std::sync::Once;
 
 use gdal_sys::{self, CPLErr, GDALDriverH, GDALMajorObjectH};
-use libc::c_int;
 
 use crate::cpl::CslStringList;
 use crate::dataset::Dataset;
@@ -105,9 +104,9 @@ impl Driver {
     pub fn create<P: AsRef<Path>>(
         &self,
         filename: P,
-        size_x: isize,
-        size_y: isize,
-        bands: isize,
+        size_x: usize,
+        size_y: usize,
+        bands: usize,
     ) -> Result<Dataset> {
         self.create_with_band_type::<u8, _>(filename, size_x, size_y, bands)
     }
@@ -134,9 +133,9 @@ impl Driver {
     pub fn create_with_band_type<T: GdalType, P: AsRef<Path>>(
         &self,
         filename: P,
-        size_x: isize,
-        size_y: isize,
-        bands: isize,
+        size_x: usize,
+        size_y: usize,
+        bands: usize,
     ) -> Result<Dataset> {
         let options = [];
         self.create_with_band_type_with_options::<T, _>(filename, size_x, size_y, bands, &options)
@@ -176,9 +175,9 @@ impl Driver {
     pub fn create_with_band_type_with_options<T: GdalType, P: AsRef<Path>>(
         &self,
         filename: P,
-        size_x: isize,
-        size_y: isize,
-        bands: isize,
+        size_x: usize,
+        size_y: usize,
+        bands: usize,
         options: &[RasterCreationOption],
     ) -> Result<Dataset> {
         Self::_create_with_band_type_with_options(
@@ -195,9 +194,9 @@ impl Driver {
     fn _create_with_band_type_with_options(
         &self,
         filename: &Path,
-        size_x: isize,
-        size_y: isize,
-        bands: isize,
+        size_x: usize,
+        size_y: usize,
+        bands: usize,
         data_type: GdalDataType,
         options: &[RasterCreationOption],
     ) -> Result<Dataset> {
@@ -206,14 +205,18 @@ impl Driver {
             options_c.set_name_value(option.key, option.value)?;
         }
 
+        let size_x = libc::c_int::try_from(size_x)?;
+        let size_y = libc::c_int::try_from(size_y)?;
+        let bands = libc::c_int::try_from(bands)?;
+
         let c_filename = _path_to_c_string(filename)?;
         let c_dataset = unsafe {
             gdal_sys::GDALCreate(
                 self.c_driver,
                 c_filename.as_ptr(),
-                size_x as c_int,
-                size_y as c_int,
-                bands as c_int,
+                size_x,
+                size_y,
+                bands,
                 data_type as u32,
                 options_c.as_ptr(),
             )

src/errors.rs

@@ -1,6 +1,7 @@
 //! GDAL Error Types
 
 use libc::c_int;
+use std::num::TryFromIntError;
 use thiserror::Error;
 
 use gdal_sys::{CPLErr, OGRErr, OGRFieldType, OGRwkbGeometryType};
@@ -82,6 +83,10 @@ pub enum GdalError {
         data_type: crate::raster::ExtendedDataTypeClass,
         method_name: &'static str,
     },
+    #[error(transparent)]
+    IntConversionError(#[from] TryFromIntError),
+    #[error("Buffer length {0} does not match raster size {1:?}")]
+    BufferSizeMismatch(usize, (usize, usize)),
 }
 
 /// A wrapper for [`CPLErr::Type`] that reflects it as an enum

src/raster/rasterband.rs

@@ -32,14 +32,11 @@ impl Dataset {
     ///
     /// # Errors
     /// Returns an error if the band cannot be read, including in the case the index is 0.
-    ///
-    /// # Panics
-    /// Panics if the band index is greater than `c_int::MAX`.
     pub fn rasterband(&self, band_index: usize) -> Result<RasterBand> {
-        assert!(band_index <= c_int::MAX as usize);
+        let band_index = libc::c_int::try_from(band_index)?;
 
         unsafe {
-            let c_band = gdal_sys::GDALGetRasterBand(self.c_dataset(), band_index as c_int);
+            let c_band = gdal_sys::GDALGetRasterBand(self.c_dataset(), band_index);
             if c_band.is_null() {
                 return Err(_last_null_pointer_err("GDALGetRasterBand"));
             }
@@ -375,7 +372,9 @@ impl<'a> RasterBand<'a> {
         e_resample_alg: Option<ResampleAlg>,
     ) -> Result<()> {
         let pixels = size.0 * size.1;
-        assert_eq!(buffer.len(), pixels);
+        if buffer.len() != pixels {
+            return Err(GdalError::BufferSizeMismatch(buffer.len(), size));
+        }
 
         let resample_alg = e_resample_alg.unwrap_or(ResampleAlg::NearestNeighbour);
 
@@ -673,14 +672,19 @@ impl<'a> RasterBand<'a> {
     /// * `window` - the window position from top left
     /// * `window_size` - the window size (GDAL will interpolate data if window_size != Buffer.size)
     /// * `buffer` - the data to write into the window
-    ///
     pub fn write<T: GdalType + Copy>(
         &mut self,
         window: (isize, isize),
         window_size: (usize, usize),
         buffer: &Buffer<T>,
     ) -> Result<()> {
-        assert_eq!(buffer.data.len(), buffer.size.0 * buffer.size.1);
+        if buffer.data.len() != buffer.size.0 * buffer.size.1 {
+            return Err(GdalError::BufferSizeMismatch(
+                buffer.data.len(),
+                buffer.size,
+            ));
+        }
+
         let rv = unsafe {
             gdal_sys::GDALRasterIO(
                 self.c_rasterband,
@@ -835,10 +839,12 @@ impl<'a> RasterBand<'a> {
         unsafe { Ok(gdal_sys::GDALGetOverviewCount(self.c_rasterband)) }
     }
 
-    pub fn overview(&self, overview_index: isize) -> Result<RasterBand<'a>> {
+    pub fn overview(&self, overview_index: usize) -> Result<RasterBand<'a>> {
+        let overview_index = libc::c_int::try_from(overview_index)?;
+
         unsafe {
             let c_band = self.c_rasterband;
-            let overview = gdal_sys::GDALGetOverview(c_band, overview_index as libc::c_int);
+            let overview = gdal_sys::GDALGetOverview(c_band, overview_index);
             if overview.is_null() {
                 return Err(_last_null_pointer_err("GDALGetOverview"));
             }
@@ -1002,21 +1008,11 @@ impl<'a> RasterBand<'a> {
     /// * `min` - Histogram lower bound
     /// * `max` - Histogram upper bound
     /// * `counts` - Histogram values for each bucket
-    ///
-    /// # Panics
-    /// Panics if the `counts.len()` is greater than `i32::MAX`.
     pub fn set_default_histogram(&self, min: f64, max: f64, counts: &mut [u64]) -> Result<()> {
-        let n_buckets = counts.len();
-        assert!(n_buckets <= i32::MAX as usize);
+        let n_buckets = libc::c_int::try_from(counts.len())?;
 
         let rv = unsafe {
-            GDALSetDefaultHistogramEx(
-                self.c_rasterband,
-                min,
-                max,
-                n_buckets as i32,
-                counts.as_mut_ptr(),
-            )
+            GDALSetDefaultHistogramEx(self.c_rasterband, min, max, n_buckets, counts.as_mut_ptr())
         };
 
         match CplErrType::from(rv) {
@@ -1048,16 +1044,15 @@ impl<'a> RasterBand<'a> {
             ));
         }
 
-        assert!(n_buckets <= i32::MAX as usize);
-
-        let mut counts = vec![0; n_buckets];
+        let n_buckets = libc::c_int::try_from(n_buckets)?;
+        let mut counts = vec![0; n_buckets as usize];
 
         let rv = unsafe {
             GDALGetRasterHistogramEx(
                 self.c_rasterband,
                 min,
                 max,
-                n_buckets as i32,
+                n_buckets,
                 counts.as_mut_ptr(),
                 libc::c_int::from(include_out_of_range),
                 libc::c_int::from(is_approx_ok),

src/vector/layer.rs

@@ -9,7 +9,7 @@ use libc::c_int;
 use std::ffi::NulError;
 use std::mem::MaybeUninit;
 use std::ptr::null_mut;
-use std::{convert::TryInto, ffi::CString, marker::PhantomData};
+use std::{ffi::CString, marker::PhantomData};
 
 use crate::errors::*;
 use crate::vector::feature::{FeatureIterator, OwnedFeatureIterator};
@@ -507,8 +507,8 @@ pub trait LayerAccess: Sized {
 
 pub struct LayerIterator<'a> {
     dataset: &'a Dataset,
-    idx: isize,
-    count: isize,
+    idx: usize,
+    count: usize,
 }
 
 impl<'a> Iterator for LayerIterator<'a> {
@@ -530,10 +530,8 @@ impl<'a> Iterator for LayerIterator<'a> {
     }
 
     fn size_hint(&self) -> (usize, Option<usize>) {
-        match Some(self.count).and_then(|s| s.try_into().ok()) {
-            Some(size) => (size, Some(size)),
-            None => (0, None),
-        }
+        let size = self.count;
+        (size, Some(size))
     }
 }
 
@@ -600,16 +598,17 @@ impl Dataset {
     }
 
     /// Get the number of layers in this dataset.
-    pub fn layer_count(&self) -> isize {
-        (unsafe { gdal_sys::OGR_DS_GetLayerCount(self.c_dataset()) }) as isize
+    pub fn layer_count(&self) -> usize {
+        (unsafe { gdal_sys::OGR_DS_GetLayerCount(self.c_dataset()) }) as usize
     }
 
     /// Fetch a layer by index.
     ///
     /// Applies to vector datasets, and fetches by the given
     /// _0-based_ index.
-    pub fn layer(&self, idx: isize) -> Result<Layer> {
-        let c_layer = unsafe { gdal_sys::OGR_DS_GetLayer(self.c_dataset(), idx as c_int) };
+    pub fn layer(&self, idx: usize) -> Result<Layer> {
+        let idx = libc::c_int::try_from(idx)?;
+        let c_layer = unsafe { gdal_sys::OGR_DS_GetLayer(self.c_dataset(), idx) };
         if c_layer.is_null() {
             return Err(_last_null_pointer_err("OGR_DS_GetLayer"));
         }
@@ -620,8 +619,9 @@ impl Dataset {
     ///
     /// Applies to vector datasets, and fetches by the given
     /// _0-based_ index.
-    pub fn into_layer(self, idx: isize) -> Result<OwnedLayer> {
-        let c_layer = unsafe { gdal_sys::OGR_DS_GetLayer(self.c_dataset(), idx as c_int) };
+    pub fn into_layer(self, idx: usize) -> Result<OwnedLayer> {
+        let idx = libc::c_int::try_from(idx)?;
+        let c_layer = unsafe { gdal_sys::OGR_DS_GetLayer(self.c_dataset(), idx) };
         if c_layer.is_null() {
             return Err(_last_null_pointer_err("OGR_DS_GetLayer"));
         }