From 9978b4e4e70dbaa2f1dfc7f0959399baee3a1cd9 Mon Sep 17 00:00:00 2001 From: Rajsekar Manokaran Date: Wed, 3 Feb 2021 03:08:57 +0530 Subject: [PATCH 01/10] Fix method self arguments --- src/dataset.rs | 10 +++++----- src/raster/rasterband.rs | 4 ++-- src/vector/layer.rs | 4 ++-- 3 files changed, 9 insertions(+), 9 deletions(-) diff --git a/src/dataset.rs b/src/dataset.rs index 08afc3d6..7d469b11 100644 --- a/src/dataset.rs +++ b/src/dataset.rs @@ -252,7 +252,7 @@ impl Dataset { } /// Set the projection reference string for this dataset. - pub fn set_projection(&self, projection: &str) -> Result<()> { + pub fn set_projection(&mut self, projection: &str) -> Result<()> { let c_projection = CString::new(projection)?; unsafe { gdal_sys::GDALSetProjection(self.c_dataset, c_projection.as_ptr()) }; Ok(()) @@ -266,7 +266,7 @@ impl Dataset { #[cfg(major_ge_3)] /// Set the spatial reference system for this dataset. - pub fn set_spatial_ref(&self, spatial_ref: &SpatialRef) -> Result<()> { + pub fn set_spatial_ref(&mut self, spatial_ref: &SpatialRef) -> Result<()> { let rv = unsafe { gdal_sys::GDALSetSpatialRef(self.c_dataset, spatial_ref.to_c_hsrs()) }; if rv != CPLErr::CE_None { return Err(_last_cpl_err(rv)); @@ -328,7 +328,7 @@ impl Dataset { /// /// Applies to vector datasets, and fetches by the given /// _0-based_ index. - pub fn layer(&mut self, idx: isize) -> Result { + pub fn layer(&self, idx: isize) -> Result { let c_layer = unsafe { gdal_sys::OGR_DS_GetLayer(self.c_dataset, idx as c_int) }; if c_layer.is_null() { return Err(_last_null_pointer_err("OGR_DS_GetLayer")); @@ -337,7 +337,7 @@ impl Dataset { } /// Fetch a layer by name. - pub fn layer_by_name(&mut self, name: &str) -> Result { + pub fn layer_by_name(&self, name: &str) -> Result { let c_name = CString::new(name)?; let c_layer = unsafe { gdal_sys::OGR_DS_GetLayerByName(self.c_dataset(), c_name.as_ptr()) }; if c_layer.is_null() { @@ -403,7 +403,7 @@ impl Dataset { /// y-coordinate of the top-left corner pixel /// column rotation (typically zero) /// height of a pixel (y-resolution, typically negative) - pub fn set_geo_transform(&self, transformation: &GeoTransform) -> Result<()> { + pub fn set_geo_transform(&mut self, transformation: &GeoTransform) -> Result<()> { assert_eq!(transformation.len(), 6); let rv = unsafe { gdal_sys::GDALSetGeoTransform(self.c_dataset, transformation.as_ptr() as *mut f64) diff --git a/src/raster/rasterband.rs b/src/raster/rasterband.rs index 3f1a8435..de999ebe 100644 --- a/src/raster/rasterband.rs +++ b/src/raster/rasterband.rs @@ -208,7 +208,7 @@ 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) pub fn write( - &self, + &mut self, window: (isize, isize), window_size: (usize, usize), buffer: &Buffer, @@ -253,7 +253,7 @@ impl<'a> RasterBand<'a> { } /// Set the no data value of this band. - pub fn set_no_data_value(&self, no_data: f64) -> Result<()> { + pub fn set_no_data_value(&mut self, no_data: f64) -> Result<()> { let rv = unsafe { gdal_sys::GDALSetRasterNoDataValue(self.c_rasterband, no_data) }; if rv != CPLErr::CE_None { return Err(_last_cpl_err(rv)); diff --git a/src/vector/layer.rs b/src/vector/layer.rs index ad32664d..0abf9b62 100644 --- a/src/vector/layer.rs +++ b/src/vector/layer.rs @@ -87,11 +87,11 @@ impl<'a> Layer<'a> { FeatureIterator::_with_layer(self) } - pub fn set_spatial_filter(&self, geometry: &Geometry) { + pub fn set_spatial_filter(&mut self, geometry: &Geometry) { unsafe { gdal_sys::OGR_L_SetSpatialFilter(self.c_layer, geometry.c_geometry()) }; } - pub fn clear_spatial_filter(&self) { + pub fn clear_spatial_filter(&mut self) { unsafe { gdal_sys::OGR_L_SetSpatialFilter(self.c_layer, null_mut()) }; } From 5aa41a801f8f256b039e00ee9c74a04e34d6f495 Mon Sep 17 00:00:00 2001 From: Rajsekar Manokaran Date: Thu, 4 Feb 2021 21:18:17 +0530 Subject: [PATCH 02/10] Fix tests --- examples/read_write_ogr.rs | 2 +- src/raster/tests.rs | 6 +++--- src/vector/vector_tests/mod.rs | 22 +++++++++++----------- 3 files changed, 15 insertions(+), 15 deletions(-) diff --git a/examples/read_write_ogr.rs b/examples/read_write_ogr.rs index dd8e64a2..5c325bd8 100644 --- a/examples/read_write_ogr.rs +++ b/examples/read_write_ogr.rs @@ -6,7 +6,7 @@ use std::fs; use std::path::Path; fn run() -> Result<()> { - let mut dataset_a = Dataset::open(Path::new("fixtures/roads.geojson"))?; + let dataset_a = Dataset::open(Path::new("fixtures/roads.geojson"))?; let layer_a = dataset_a.layer(0)?; let fields_defn = layer_a .defn() diff --git a/src/raster/tests.rs b/src/raster/tests.rs index 00e50807..374819f3 100644 --- a/src/raster/tests.rs +++ b/src/raster/tests.rs @@ -96,7 +96,7 @@ fn test_write_raster() { }; // epand it to fill the image (20x10) - let rb = dataset.rasterband(1).unwrap(); + let mut rb = dataset.rasterband(1).unwrap(); let res = rb.write((0, 0), (20, 10), &raster); @@ -218,7 +218,7 @@ fn test_create_copy() { #[allow(clippy::float_cmp)] fn test_geo_transform() { let driver = Driver::get("MEM").unwrap(); - let dataset = driver.create("", 20, 10, 1).unwrap(); + let mut dataset = driver.create("", 20, 10, 1).unwrap(); let transform = [0., 1., 0., 0., 0., 1.]; assert!(dataset.set_geo_transform(&transform).is_ok()); assert_eq!(dataset.geo_transform().unwrap(), transform); @@ -360,7 +360,7 @@ fn test_get_no_data_value() { fn test_set_no_data_value() { let driver = Driver::get("MEM").unwrap(); let dataset = driver.create("", 20, 10, 1).unwrap(); - let rasterband = dataset.rasterband(1).unwrap(); + let mut rasterband = dataset.rasterband(1).unwrap(); assert_eq!(rasterband.no_data_value(), None); assert!(rasterband.set_no_data_value(1.23).is_ok()); assert_eq!(rasterband.no_data_value(), Some(1.23)); diff --git a/src/vector/vector_tests/mod.rs b/src/vector/vector_tests/mod.rs index 07f35fd0..badfba32 100644 --- a/src/vector/vector_tests/mod.rs +++ b/src/vector/vector_tests/mod.rs @@ -39,7 +39,7 @@ fn test_many_layer_count() { #[test] fn test_layer_get_extent() { - let mut ds = Dataset::open(fixture!("roads.geojson")).unwrap(); + let ds = Dataset::open(fixture!("roads.geojson")).unwrap(); let layer = ds.layer(0).unwrap(); let extent = layer.get_extent().unwrap(); assert_almost_eq(extent.MinX, 26.100768); @@ -50,14 +50,14 @@ fn test_layer_get_extent() { #[test] fn test_layer_try_get_extent() { - let mut ds = Dataset::open(fixture!("roads.geojson")).unwrap(); + let ds = Dataset::open(fixture!("roads.geojson")).unwrap(); let layer = ds.layer(0).unwrap(); assert!(layer.try_get_extent().unwrap().is_none()); } #[test] fn test_layer_spatial_ref() { - let mut ds = Dataset::open(fixture!("roads.geojson")).unwrap(); + let ds = Dataset::open(fixture!("roads.geojson")).unwrap(); let layer = ds.layer(0).unwrap(); let srs = layer.spatial_ref().unwrap(); assert_eq!(srs.auth_code().unwrap(), 4326); @@ -67,7 +67,7 @@ fn ds_with_layer(ds_name: &str, layer_name: &str, f: F) where F: Fn(Layer), { - let mut ds = Dataset::open(fixture!(ds_name)).unwrap(); + let ds = Dataset::open(fixture!(ds_name)).unwrap(); let layer = ds.layer_by_name(layer_name).unwrap(); f(layer); } @@ -76,7 +76,7 @@ fn with_layer(name: &str, f: F) where F: Fn(Layer), { - let mut ds = Dataset::open(fixture!(name)).unwrap(); + let ds = Dataset::open(fixture!(name)).unwrap(); let layer = ds.layer(0).unwrap(); f(layer); } @@ -323,7 +323,7 @@ mod tests { #[test] fn test_schema() { - let mut ds = Dataset::open(fixture!("roads.geojson")).unwrap(); + let ds = Dataset::open(fixture!("roads.geojson")).unwrap(); let layer = ds.layer(0).unwrap(); // The layer name is "roads" in GDAL 2.2 assert!(layer.name() == "OGRGeoJSON" || layer.name() == "roads"); @@ -349,7 +349,7 @@ mod tests { #[test] fn test_geom_fields() { - let mut ds = Dataset::open(fixture!("roads.geojson")).unwrap(); + let ds = Dataset::open(fixture!("roads.geojson")).unwrap(); let layer = ds.layer(0).unwrap(); let name_list = layer .defn() @@ -372,7 +372,7 @@ mod tests { #[test] fn test_get_layer_by_name() { - let mut ds = Dataset::open(fixture!("roads.geojson")).unwrap(); + let ds = Dataset::open(fixture!("roads.geojson")).unwrap(); // The layer name is "roads" in GDAL 2.2 if let Ok(layer) = ds.layer_by_name("OGRGeoJSON") { assert_eq!(layer.name(), "OGRGeoJSON"); @@ -390,8 +390,8 @@ mod tests { #[test] fn test_spatial_filter() { - let mut ds = Dataset::open(fixture!("roads.geojson")).unwrap(); - let layer = ds.layer(0).unwrap(); + let ds = Dataset::open(fixture!("roads.geojson")).unwrap(); + let mut layer = ds.layer(0).unwrap(); assert_eq!(layer.features().count(), 21); let bbox = Geometry::bbox(26.1017, 44.4297, 26.1025, 44.4303).unwrap(); @@ -479,7 +479,7 @@ mod tests { // dataset is closed here } - let mut ds = Dataset::open(fixture!("output.geojson")).unwrap(); + let ds = Dataset::open(fixture!("output.geojson")).unwrap(); fs::remove_file(fixture!("output.geojson")).unwrap(); let layer = ds.layer(0).unwrap(); let ft = layer.features().next().unwrap(); From b2eec35cbdf797ae45a5a6bb31bff279174b1f89 Mon Sep 17 00:00:00 2001 From: Rajsekar Manokaran Date: Thu, 4 Feb 2021 21:27:59 +0530 Subject: [PATCH 03/10] Make features iterator take mut ref Refer: https://github.com/georust/gdal/issues/159 --- examples/read_write_ogr.rs | 2 +- src/lib.rs | 4 ++-- src/vector/layer.rs | 28 +++++++++++++++------------- src/vector/mod.rs | 4 ++-- src/vector/vector_tests/mod.rs | 22 +++++++++++++++++++--- 5 files changed, 39 insertions(+), 21 deletions(-) diff --git a/examples/read_write_ogr.rs b/examples/read_write_ogr.rs index 5c325bd8..57f3c500 100644 --- a/examples/read_write_ogr.rs +++ b/examples/read_write_ogr.rs @@ -7,7 +7,7 @@ use std::path::Path; fn run() -> Result<()> { let dataset_a = Dataset::open(Path::new("fixtures/roads.geojson"))?; - let layer_a = dataset_a.layer(0)?; + let mut layer_a = dataset_a.layer(0)?; let fields_defn = layer_a .defn() .fields() diff --git a/src/lib.rs b/src/lib.rs index 11017979..8dbb7875 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -8,8 +8,8 @@ //! use std::path::Path; //! use gdal::Dataset; //! -//! let mut dataset = Dataset::open(Path::new("fixtures/roads.geojson")).unwrap(); -//! let layer = dataset.layer(0).unwrap(); +//! let dataset = Dataset::open(Path::new("fixtures/roads.geojson")).unwrap(); +//! let mut layer = dataset.layer(0).unwrap(); //! for feature in layer.features() { //! let highway_field = feature.field("highway").unwrap().unwrap(); //! let geometry = feature.geometry(); diff --git a/src/vector/layer.rs b/src/vector/layer.rs index 0abf9b62..614cf3cb 100644 --- a/src/vector/layer.rs +++ b/src/vector/layer.rs @@ -19,8 +19,8 @@ use crate::errors::*; /// use std::path::Path; /// use gdal::Dataset; /// -/// let mut dataset = Dataset::open(Path::new("fixtures/roads.geojson")).unwrap(); -/// let layer = dataset.layer(0).unwrap(); +/// let dataset = Dataset::open(Path::new("fixtures/roads.geojson")).unwrap(); +/// let mut layer = dataset.layer(0).unwrap(); /// for feature in layer.features() { /// // do something with each feature /// } @@ -83,7 +83,7 @@ impl<'a> Layer<'a> { } /// Iterate over all features in this layer. - pub fn features(&self) -> FeatureIterator { + pub fn features(&mut self) -> FeatureIterator { FeatureIterator::_with_layer(self) } @@ -244,7 +244,9 @@ impl<'a> Layer<'a> { } pub struct FeatureIterator<'a> { - layer: &'a Layer<'a>, + defn: &'a Defn, + c_layer: OGRLayerH, + size_hint: Option, } impl<'a> Iterator for FeatureIterator<'a> { @@ -252,21 +254,16 @@ impl<'a> Iterator for FeatureIterator<'a> { #[inline] fn next(&mut self) -> Option> { - let c_feature = unsafe { gdal_sys::OGR_L_GetNextFeature(self.layer.c_layer) }; + let c_feature = unsafe { gdal_sys::OGR_L_GetNextFeature(self.c_layer) }; if c_feature.is_null() { None } else { - Some(unsafe { Feature::from_c_feature(self.layer.defn(), c_feature) }) + Some(unsafe { Feature::from_c_feature(self.defn, c_feature) }) } } fn size_hint(&self) -> (usize, Option) { - match self - .layer - .try_feature_count() - .map(|s| s.try_into().ok()) - .flatten() - { + match self.size_hint { Some(size) => (size, Some(size)), None => (0, None), } @@ -275,7 +272,12 @@ impl<'a> Iterator for FeatureIterator<'a> { impl<'a> FeatureIterator<'a> { pub fn _with_layer(layer: &'a Layer) -> FeatureIterator<'a> { - FeatureIterator { layer } + let defn = layer.defn(); + let size_hint = layer + .try_feature_count() + .map(|s| s.try_into().ok()) + .flatten(); + FeatureIterator { c_layer: layer.c_layer, size_hint, defn } } } diff --git a/src/vector/mod.rs b/src/vector/mod.rs index 4c6b9e90..2daa1ae0 100644 --- a/src/vector/mod.rs +++ b/src/vector/mod.rs @@ -6,8 +6,8 @@ //! use std::path::Path; //! use gdal::Dataset; //! -//! let mut dataset = Dataset::open(Path::new("fixtures/roads.geojson")).unwrap(); -//! let layer = dataset.layer(0).unwrap(); +//! let dataset = Dataset::open(Path::new("fixtures/roads.geojson")).unwrap(); +//! let mut layer = dataset.layer(0).unwrap(); //! for feature in layer.features() { //! let highway_field = feature.field("highway").unwrap().unwrap(); //! let geometry = feature.geometry(); diff --git a/src/vector/vector_tests/mod.rs b/src/vector/vector_tests/mod.rs index badfba32..c3a0510b 100644 --- a/src/vector/vector_tests/mod.rs +++ b/src/vector/vector_tests/mod.rs @@ -85,7 +85,7 @@ fn with_features(name: &str, f: F) where F: Fn(FeatureIterator), { - with_layer(name, |layer| f(layer.features())); + with_layer(name, |mut layer| f(layer.features())); } fn with_feature(name: &str, fid: u64, f: F) @@ -205,7 +205,7 @@ mod tests { #[test] fn test_field_in_layer() { - ds_with_layer("three_layer_ds.s3db", "layer_0", |layer| { + ds_with_layer("three_layer_ds.s3db", "layer_0", |mut layer| { let feature = layer.features().next().unwrap(); assert_eq!(feature.field("id").unwrap(), None); }); @@ -481,7 +481,7 @@ mod tests { let ds = Dataset::open(fixture!("output.geojson")).unwrap(); fs::remove_file(fixture!("output.geojson")).unwrap(); - let layer = ds.layer(0).unwrap(); + let mut layer = ds.layer(0).unwrap(); let ft = layer.features().next().unwrap(); assert_eq!(ft.geometry().wkt().unwrap(), "POINT (1 2)"); assert_eq!( @@ -491,4 +491,20 @@ mod tests { assert_eq!(ft.field("Value").unwrap().unwrap().into_real(), Some(45.78)); assert_eq!(ft.field("Int_value").unwrap().unwrap().into_int(), Some(1)); } + + // A compilation test that should fail. + // + // It tries to iterate over a layer's features, while + // also trying to read the layer's definition. The + // features iterator borrows layer as a mutable ref as + // it increments the internal state of the layer. + // + // fn test_features_mut_lifetime_enforce() { + // with_layer("roads.geojson", |mut layer| { + // for _ in layer.features() { + // let _ = layer.defn(); + // } + // }); + // } + } From 86f02f3aacb74b1176c45434748723282f665222 Mon Sep 17 00:00:00 2001 From: Rajsekar Manokaran Date: Thu, 4 Feb 2021 21:56:51 +0530 Subject: [PATCH 04/10] Make features iterator reset next feature pointer --- src/vector/layer.rs | 7 +++++++ src/vector/vector_tests/mod.rs | 11 +++++++++++ 2 files changed, 18 insertions(+) diff --git a/src/vector/layer.rs b/src/vector/layer.rs index 614cf3cb..bdb32088 100644 --- a/src/vector/layer.rs +++ b/src/vector/layer.rs @@ -84,6 +84,7 @@ impl<'a> Layer<'a> { /// Iterate over all features in this layer. pub fn features(&mut self) -> FeatureIterator { + self.reset_feature_reading(); FeatureIterator::_with_layer(self) } @@ -241,6 +242,12 @@ impl<'a> Layer<'a> { } SpatialRef::from_c_obj(c_obj) } + + fn reset_feature_reading(&mut self) { + unsafe { + gdal_sys::OGR_L_ResetReading(self.c_layer); + } + } } pub struct FeatureIterator<'a> { diff --git a/src/vector/vector_tests/mod.rs b/src/vector/vector_tests/mod.rs index c3a0510b..4247d5a1 100644 --- a/src/vector/vector_tests/mod.rs +++ b/src/vector/vector_tests/mod.rs @@ -492,6 +492,17 @@ mod tests { assert_eq!(ft.field("Int_value").unwrap().unwrap().into_int(), Some(1)); } + #[test] + fn test_features_reset() { + with_layer("roads.geojson", |mut layer| { + assert_eq!( + layer.features().count(), + layer.features().count(), + ); + }); + + } + // A compilation test that should fail. // // It tries to iterate over a layer's features, while From 0e5d276a183428f39f3fb7bdd7394b53f9945d0c Mon Sep 17 00:00:00 2001 From: Rajsekar Manokaran Date: Thu, 4 Feb 2021 22:10:46 +0530 Subject: [PATCH 05/10] Cargo fmt --- src/vector/layer.rs | 6 +++++- src/vector/vector_tests/mod.rs | 7 +------ 2 files changed, 6 insertions(+), 7 deletions(-) diff --git a/src/vector/layer.rs b/src/vector/layer.rs index bdb32088..5c23499c 100644 --- a/src/vector/layer.rs +++ b/src/vector/layer.rs @@ -284,7 +284,11 @@ impl<'a> FeatureIterator<'a> { .try_feature_count() .map(|s| s.try_into().ok()) .flatten(); - FeatureIterator { c_layer: layer.c_layer, size_hint, defn } + FeatureIterator { + c_layer: layer.c_layer, + size_hint, + defn, + } } } diff --git a/src/vector/vector_tests/mod.rs b/src/vector/vector_tests/mod.rs index 4247d5a1..a307e172 100644 --- a/src/vector/vector_tests/mod.rs +++ b/src/vector/vector_tests/mod.rs @@ -495,12 +495,8 @@ mod tests { #[test] fn test_features_reset() { with_layer("roads.geojson", |mut layer| { - assert_eq!( - layer.features().count(), - layer.features().count(), - ); + assert_eq!(layer.features().count(), layer.features().count(),); }); - } // A compilation test that should fail. @@ -517,5 +513,4 @@ mod tests { // } // }); // } - } From d55643edbda94dd41ec5a917affad99ba50970ba Mon Sep 17 00:00:00 2001 From: Rajsekar Manokaran Date: Thu, 4 Feb 2021 22:23:29 +0530 Subject: [PATCH 06/10] Fix compilation errors on all-features --- examples/read_write_ogr_datetime.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/examples/read_write_ogr_datetime.rs b/examples/read_write_ogr_datetime.rs index c6bbd486..972d3f6c 100644 --- a/examples/read_write_ogr_datetime.rs +++ b/examples/read_write_ogr_datetime.rs @@ -10,8 +10,8 @@ fn run() -> Result<()> { println!("gdal crate was build with datetime support"); - let mut dataset_a = Dataset::open(Path::new("fixtures/points_with_datetime.json"))?; - let layer_a = dataset_a.layer(0)?; + let dataset_a = Dataset::open(Path::new("fixtures/points_with_datetime.json"))?; + let mut layer_a = dataset_a.layer(0)?; // Create a new dataset: let path = std::env::temp_dir().join("later.geojson"); From 44c78ae0966788dc52359b8753a4b0b60274f9dc Mon Sep 17 00:00:00 2001 From: Rajsekar Manokaran Date: Fri, 5 Feb 2021 02:23:26 +0530 Subject: [PATCH 07/10] Add compile tests to ensure borrow semantics + add `trybuild` + add test to verify `Layer::features` mut-borrow. Refer https://github.com/georust/gdal/issues/159 --- Cargo.toml | 1 + src/vector/layer.rs | 17 ++++++++++++----- src/vector/vector_tests/mod.rs | 8 ++++++-- tests/ui/01-features-aliasing-errors.rs | 11 +++++++++++ tests/ui/01-features-aliasing-errors.stderr | 10 ++++++++++ tests/ui/utils.rs | 14 ++++++++++++++ 6 files changed, 54 insertions(+), 7 deletions(-) create mode 100644 tests/ui/01-features-aliasing-errors.rs create mode 100644 tests/ui/01-features-aliasing-errors.stderr create mode 100644 tests/ui/utils.rs diff --git a/Cargo.toml b/Cargo.toml index 9744a83d..48872c21 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -31,6 +31,7 @@ semver = "0.11" [dev-dependencies] tempfile = "3.1" +trybuild = "1.0.39" [workspace] members = ["gdal-sys"] diff --git a/src/vector/layer.rs b/src/vector/layer.rs index 5c23499c..5a65c4ac 100644 --- a/src/vector/layer.rs +++ b/src/vector/layer.rs @@ -69,10 +69,6 @@ impl<'a> Layer<'a> { /// Not all drivers support this efficiently; however, the call should always work if the /// feature exists, as a fallback implementation just scans all the features in the layer /// looking for the desired feature. - /// - /// **Important**: do not call `feature` while iterating over features using the - /// [`features`](Layer::features) function. Even if you don't mutate anything, this can cause - /// the the iterator to get interrupted. This is a limitation of the GDAL library. pub fn feature(&self, fid: u64) -> Option { let c_feature = unsafe { gdal_sys::OGR_L_GetFeature(self.c_layer, fid as i64) }; if c_feature.is_null() { @@ -82,16 +78,24 @@ impl<'a> Layer<'a> { } } - /// Iterate over all features in this layer. + /// Returns iterator over the features in this layer. + /// + /// **Note.** This method resets the current index to + /// the beginning before iteration. It also borrows the + /// layer mutably, preventing any overlapping borrows. pub fn features(&mut self) -> FeatureIterator { self.reset_feature_reading(); FeatureIterator::_with_layer(self) } + /// Set a spatial filter on this layer. + /// + /// Refer [OGR_L_SetSpatialFilter](https://gdal.org/doxygen/classOGRLayer.html#a75c06b4993f8eb76b569f37365cd19ab) pub fn set_spatial_filter(&mut self, geometry: &Geometry) { unsafe { gdal_sys::OGR_L_SetSpatialFilter(self.c_layer, geometry.c_geometry()) }; } + /// Clear spatial filters set on this layer. pub fn clear_spatial_filter(&mut self) { unsafe { gdal_sys::OGR_L_SetSpatialFilter(self.c_layer, null_mut()) }; } @@ -235,6 +239,9 @@ impl<'a> Layer<'a> { } } + /// Fetch the spatial reference system for this layer. + /// + /// Refer [OGR_L_GetSpatialRef](https://gdal.org/doxygen/classOGRLayer.html#a75c06b4993f8eb76b569f37365cd19ab) pub fn spatial_ref(&self) -> Result { let c_obj = unsafe { gdal_sys::OGR_L_GetSpatialRef(self.c_layer) }; if c_obj.is_null() { diff --git a/src/vector/vector_tests/mod.rs b/src/vector/vector_tests/mod.rs index a307e172..ea964c8c 100644 --- a/src/vector/vector_tests/mod.rs +++ b/src/vector/vector_tests/mod.rs @@ -499,8 +499,12 @@ mod tests { }); } - // A compilation test that should fail. - // + #[test] + fn test_features_aliasing_compile_fail() { + let t = trybuild::TestCases::new(); // A compilation test that should fail. + t.compile_fail("tests/ui/01-features-aliasing-errors.rs"); + } + // It tries to iterate over a layer's features, while // also trying to read the layer's definition. The // features iterator borrows layer as a mutable ref as diff --git a/tests/ui/01-features-aliasing-errors.rs b/tests/ui/01-features-aliasing-errors.rs new file mode 100644 index 00000000..eb531f12 --- /dev/null +++ b/tests/ui/01-features-aliasing-errors.rs @@ -0,0 +1,11 @@ +mod utils; + +use gdal::Dataset; + +fn main() { + let ds = Dataset::open(fixture!("roads.geojson")).unwrap(); + let mut layer = ds.layer(0).unwrap(); + for _ in layer.features() { + let _ = layer.defn(); + } +} diff --git a/tests/ui/01-features-aliasing-errors.stderr b/tests/ui/01-features-aliasing-errors.stderr new file mode 100644 index 00000000..016a7356 --- /dev/null +++ b/tests/ui/01-features-aliasing-errors.stderr @@ -0,0 +1,10 @@ +error[E0502]: cannot borrow `layer` as immutable because it is also borrowed as mutable + --> $DIR/01-features-aliasing-errors.rs:9:17 + | +8 | for _ in layer.features() { + | ---------------- + | | + | mutable borrow occurs here + | mutable borrow later used here +9 | let _ = layer.defn(); + | ^^^^^ immutable borrow occurs here diff --git a/tests/ui/utils.rs b/tests/ui/utils.rs new file mode 100644 index 00000000..29e6368a --- /dev/null +++ b/tests/ui/utils.rs @@ -0,0 +1,14 @@ +#[macro_export] +macro_rules! fixture { + ($name:expr) => { + std::path::Path::new(file!()) + .parent() + .unwrap() + .parent() + .unwrap() + .join("fixtures") + .as_path() + .join($name) + .as_path() + }; +} From a3a652455bdc1828cecd9bfc5b51c07bdbcbb8b3 Mon Sep 17 00:00:00 2001 From: Rajsekar Manokaran Date: Fri, 5 Feb 2021 03:07:47 +0530 Subject: [PATCH 08/10] Add changes --- CHANGES.md | 27 ++++++++++++++++++++++++--- 1 file changed, 24 insertions(+), 3 deletions(-) diff --git a/CHANGES.md b/CHANGES.md index e24c393b..bd1211a1 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -1,6 +1,27 @@ # Changes ## Unreleased + +* **Breaking**: Make `Layer::features` iterator reset to + beginning, and borrow mutably. + * closes + +* **Breaking**: [Enforce borrow + semantics](https://github.com/georust/gdal/pull/161) on + methods of `Dataset`, `RasterBand`, and `Layer`. + 1. Methods that do not modify the underlying structure take `&self`. + 1. Methods that modify the underlying structure take `&mut self`. + + ```rust + let ds = Dataset::open(...); + + // ds need not be mutable to open layer + let mut band = ds.rasterband(1)?; + + // band needs to be mutable to set no-data value + band.set_no_data_value(0.0)?; + ``` + * **Breaking**: Use `DatasetOptions` to pass as `Dataset::open_ex` parameters and add support for extended open flags. @@ -9,7 +30,7 @@ let dataset = Dataset::open_ex( "roads.geojson", - DatasetOptions { + DatasetOptions { open_flags: GdalOpenFlags::GDAL_OF_UPDATE|GdalOpenFlags::GDAL_OF_VECTOR, ..DatasetOptions::default() } @@ -17,7 +38,7 @@ .unwrap(); ``` - `GDALAccess` values are supported usinf [`From`] implementation + `GDALAccess` values are supported using [`From`] implementation ```rust Dataset::open_ex( @@ -31,7 +52,7 @@ ``` * Add more functions to SpatialRef implementation - * + * * **Breaking**: Change `Feature::field` return type from `Result` to `Result>`. Fields can be null. Before this change, if a field was null, the value From de721a4363cfd1c7fb1b102b25208933994aec6f Mon Sep 17 00:00:00 2001 From: Rajsekar Manokaran Date: Tue, 23 Feb 2021 16:54:48 +0530 Subject: [PATCH 09/10] Rename compile test folder more meaningfully --- src/vector/vector_tests/mod.rs | 2 +- tests/{ui => compile-tests}/01-features-aliasing-errors.rs | 0 tests/{ui => compile-tests}/01-features-aliasing-errors.stderr | 0 tests/{ui => compile-tests}/utils.rs | 0 4 files changed, 1 insertion(+), 1 deletion(-) rename tests/{ui => compile-tests}/01-features-aliasing-errors.rs (100%) rename tests/{ui => compile-tests}/01-features-aliasing-errors.stderr (100%) rename tests/{ui => compile-tests}/utils.rs (100%) diff --git a/src/vector/vector_tests/mod.rs b/src/vector/vector_tests/mod.rs index ea964c8c..ca06bb16 100644 --- a/src/vector/vector_tests/mod.rs +++ b/src/vector/vector_tests/mod.rs @@ -502,7 +502,7 @@ mod tests { #[test] fn test_features_aliasing_compile_fail() { let t = trybuild::TestCases::new(); // A compilation test that should fail. - t.compile_fail("tests/ui/01-features-aliasing-errors.rs"); + t.compile_fail("tests/compile-tests/01-features-aliasing-errors.rs"); } // It tries to iterate over a layer's features, while diff --git a/tests/ui/01-features-aliasing-errors.rs b/tests/compile-tests/01-features-aliasing-errors.rs similarity index 100% rename from tests/ui/01-features-aliasing-errors.rs rename to tests/compile-tests/01-features-aliasing-errors.rs diff --git a/tests/ui/01-features-aliasing-errors.stderr b/tests/compile-tests/01-features-aliasing-errors.stderr similarity index 100% rename from tests/ui/01-features-aliasing-errors.stderr rename to tests/compile-tests/01-features-aliasing-errors.stderr diff --git a/tests/ui/utils.rs b/tests/compile-tests/utils.rs similarity index 100% rename from tests/ui/utils.rs rename to tests/compile-tests/utils.rs From 52951a1b5d0e1a605bfd1a4c400b7ee6606660a0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Johannes=20Dr=C3=B6nner?= Date: Sun, 25 Apr 2021 12:54:36 +0200 Subject: [PATCH 10/10] DerefMut for ResultSet --- src/dataset.rs | 2 +- src/vector/sql.rs | 8 +++++++- src/vector/vector_tests/mod.rs | 2 +- src/vector/vector_tests/sql.rs | 8 ++++---- 4 files changed, 13 insertions(+), 7 deletions(-) diff --git a/src/dataset.rs b/src/dataset.rs index c6f99df0..3ef59d24 100644 --- a/src/dataset.rs +++ b/src/dataset.rs @@ -567,7 +567,7 @@ impl Dataset { /// /// let ds = Dataset::open(Path::new("fixtures/roads.geojson")).unwrap(); /// let query = "SELECT kind, is_bridge, highway FROM roads WHERE highway = 'pedestrian'"; - /// let result_set = ds.execute_sql(query, None, sql::Dialect::DEFAULT).unwrap().unwrap(); + /// let mut result_set = ds.execute_sql(query, None, sql::Dialect::DEFAULT).unwrap().unwrap(); /// /// assert_eq!(10, result_set.feature_count()); /// diff --git a/src/vector/sql.rs b/src/vector/sql.rs index decd23d5..ef7d717d 100644 --- a/src/vector/sql.rs +++ b/src/vector/sql.rs @@ -1,4 +1,4 @@ -use std::ops::Deref; +use std::ops::{Deref, DerefMut}; use gdal_sys::GDALDatasetH; @@ -21,6 +21,12 @@ impl<'a> Deref for ResultSet<'a> { } } +impl<'a> DerefMut for ResultSet<'a> { + fn deref_mut(&mut self) -> &mut ::Target { + &mut self.layer + } +} + impl<'a> Drop for ResultSet<'a> { fn drop(&mut self) { unsafe { gdal_sys::GDALDatasetReleaseResultSet(self.dataset, self.layer.c_layer()) }; diff --git a/src/vector/vector_tests/mod.rs b/src/vector/vector_tests/mod.rs index b6845135..2231a13f 100644 --- a/src/vector/vector_tests/mod.rs +++ b/src/vector/vector_tests/mod.rs @@ -67,7 +67,7 @@ fn test_layer_spatial_ref() { #[test] fn test_layer_capabilities() { - let mut ds = Dataset::open(fixture!("roads.geojson")).unwrap(); + let ds = Dataset::open(fixture!("roads.geojson")).unwrap(); let layer = ds.layer(0).unwrap(); assert!(!layer.has_capability(OLCFastSpatialFilter)); diff --git a/src/vector/vector_tests/sql.rs b/src/vector/vector_tests/sql.rs index 82fffc51..34e0d198 100644 --- a/src/vector/vector_tests/sql.rs +++ b/src/vector/vector_tests/sql.rs @@ -10,7 +10,7 @@ use crate::{ fn test_sql() { let ds = Dataset::open(fixture!("roads.geojson")).unwrap(); let query = "SELECT kind, is_bridge, highway FROM roads WHERE highway = 'pedestrian'"; - let result_set = ds + let mut result_set = ds .execute_sql(query, None, sql::Dialect::DEFAULT) .unwrap() .unwrap(); @@ -46,7 +46,7 @@ fn test_sql_with_spatial_filter() { let query = "SELECT * FROM roads WHERE highway = 'pedestrian'"; let ds = Dataset::open(fixture!("roads.geojson")).unwrap(); let bbox = Geometry::bbox(26.1017, 44.4297, 26.1025, 44.4303).unwrap(); - let result_set = ds + let mut result_set = ds .execute_sql(query, Some(&bbox), sql::Dialect::DEFAULT) .unwrap() .unwrap(); @@ -77,7 +77,7 @@ fn test_sql_with_dialect() { let query = "SELECT * FROM roads WHERE highway = 'pedestrian' and NumPoints(GEOMETRY) = 3"; let ds = Dataset::open(fixture!("roads.geojson")).unwrap(); let bbox = Geometry::bbox(26.1017, 44.4297, 26.1025, 44.4303).unwrap(); - let result_set = ds + let mut result_set = ds .execute_sql(query, Some(&bbox), sql::Dialect::SQLITE) .unwrap() .unwrap(); @@ -99,7 +99,7 @@ fn test_sql_with_dialect() { fn test_sql_empty_result() { let ds = Dataset::open(fixture!("roads.geojson")).unwrap(); let query = "SELECT kind, is_bridge, highway FROM roads WHERE highway = 'jazz hands 👐'"; - let result_set = ds + let mut result_set = ds .execute_sql(query, None, sql::Dialect::DEFAULT) .unwrap() .unwrap();