-
Notifications
You must be signed in to change notification settings - Fork 94
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
A Consistent and Principled Approach to GDAL C Pointers #445
Conversation
1974267
to
c2e29cc
Compare
447: Maintenance cleanup on `dataset.rs` r=lnicola a=metasim - [X] I agree to follow the project's [code of conduct](https://github.com/georust/gdal/blob/master/CODE_OF_CONDUCT.md). - [X] I added an entry to `CHANGES.md` if knowledge of this change could be valuable to users. --- Changes: * Moved ancillary types from `dataset.rs` into their own or related files. * Moved non-core Dataset methods into respective `raster` or `vector` modules. Aims to simplify maintainability via "separation of concerns". * Removed `unsafe` from pointer accessor methods. _Possibly_ breaking changes: * Moved `LayerIterator`, `LayerOptions` and `Transaction` to `crate::vector`. --- Aside: The impetus for this refactor is to focus the changes necessary for adding `Dataset` to #445. But even if that PR takes a different route, this change will benefit long-term maintenance. Co-authored-by: Simeon H.K. Fitch <[email protected]>
<MDI key="STATISTICS_STDDEV">83.68444773935</MDI> | ||
<MDI key="STATISTICS_VALID_PERCENT">100</MDI> | ||
</Metadata> | ||
</PAMRasterBand> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll revert this.
c2e29cc
to
7db2868
Compare
src/raster/rasterband.rs
Outdated
@@ -25,13 +26,13 @@ impl Dataset { | |||
/// | |||
/// Applies to raster datasets, and fetches the | |||
/// rasterband at the given _1-based_ index. | |||
pub fn rasterband(&self, band_index: isize) -> Result<RasterBand> { | |||
pub fn rasterband(&self, band_index: isize) -> Result<&mut RasterBand> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will need to change. Semantically, it's no different from what we have now where you can get a RasterBand
from a non-mut
Dataset
, and then mutate it. This just makes it more clear that (even now) you can get multiple mutable references to the same band data. See PR notes on rasterband_mut
.
a2b3614
to
0b010cd
Compare
0b010cd
to
2f1a8f1
Compare
@@ -153,7 +154,7 @@ impl Dataset { | |||
|
|||
Ok(Some(sql::ResultSet { | |||
layer, | |||
dataset: c_dataset, | |||
dataset: c_dataset, // TODO: We're ending up with a shared reference here... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will need a second pair of eyes on this one.
unsafe { SpatialRef::from_c_obj(c_ptr) }.ok() | ||
// NB: Creating a `SpatialRefRef` from a pointer acknowledges that GDAL is giving us | ||
// a shared reference. We then call `to_owned` to appropriately get a clone. | ||
Some(unsafe { SpatialRefRef::from_ptr(c_ptr) }.to_owned()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lnicola said in 7db2868#r128939164
We could return a SpatialRefRef here, right?
One more reason to rename these to SpatialReference.
While I think the approaches herein are ones we should adopt, I think it's too late (with volunteer labor) to apply a unified approach across the whole library. While the porting |
Remaining Tasks:
Dataset
(dependency Maintenance cleanup ondataset.rs
#447)GCP
(possibly; it has a Rust type mirroring C type){Owned}Layer
(it's really strange)FieldDefn
mdarray::MDArray
mdarray::Group
mdarray::Dimension
mdarray::ExtendedDataType
mdarray::Attribute
SpatialRef
toSpatialReference
to avoidSpatialRefRef
vsi
,cpl
, etc.vector
module. #448)CHANGES.md
if knowledge of this change could be valuable to users.Background
The Rust bindings to the GDAL library have been developed and maintained by many contributors over a time span of almost a decade (the first commit on record is March 25, 2014). As is normal in such a situation, inconsistencies and diverging principles arise. The scope of this PR is to address inconsistencies in the handling of pointers to C-instantiated GDAL types, making use use of the
foreign-types
crate as a means.The
foreign-types
crate is mature and widely used, with 45 public dependents (includingopenssl
), approximately 120,000 daily downloads, and > 65 million total downloads. It is dual licensed under Apache and MIT licenses. It provides an opinionated set of types and traits for wrapping pointers to C memory, with support for borrowed vs owned references,Clone
andDrop
, with optional proc-macros to eliminate some boilerplate. The included documentation and examples are somewhat sparse, but the maintainer is responsive to questions. If you dig deep enough (including the very large projects that use the crate), you can find good examples. While the names and abstractions declared inforeign-types
may not be to the preferences of all users, maturity, consistency and friction-lowering were given primacy in consideringforeign-types
.Note: For simplicity I use the term "pointer" to include memory addresses, handles, or any other identifier to a C-allocated resource in GDAL.
Goals
The impetus for this PR is several logged issues around pointer handling:
c_dataset
and friends) should be safe #168This PR aims to provide foundational work helpful in addressing these issues, primarily focusing on "safe consistency". Specifically:
pub
. Becausegeorust/gdal
is (currently) a subset of the functionality covered byOSGeo/gdal
, it's important to enable advanced users to access GDAL pointers in a principled but enabling way.unsafe
: There's inconsistency in when pointer-related methods are marked asunsafe
. It is not idiomatic to treat pointer reading, manipulation or arithmetic asunsafe
. Rust considers it safe to manipulate pointers and other handles, while dereferencing or turning them back into something useable isunsafe
.c_dataset
,to_c_hsrs
,c_options
,to_c_hct
,c_geometry
, etc. to access the C pointer.foreign-types
we can lift those documented concepts into compiler-checked constraints, handling the allocation, deallocation, cloning, ownership transfer, and lifetime tracking aspects of the resource lifecycle.Benefits
The intended benefits are:
Scope
This PR migrates the following types to use
foreign-types
Defn
Feature
Geometry
SpatialRef
CoordTransform
CoordTransformOptions
RasterBand
(See Remaining Tasks below)
Note: Because of this scope, many APIs will break, necessitating clear documentation and appropriate semantic version handling (albeit we're still < 1.0).
Notes on Migration
These are some thoughts I had about the migration experience, some of which are copied from the related discussion on this topic. I include them here for those interested in the deeper deails behind certain implementation decisions.
foreign_type!
macros, but after implementingForeignType
andForeignTypeRef
manually, it's definitely worth it when it's appropriate. it is not always appropriate, such as the case forRasterBand
where we are always working with shared references (always owned byDataset
). See below.foreign-types
.ForeignType::from_ptr
, which isunsafe
, is how you create instances.ForeignType::as_ptr
, which is "safe" (as we desire here), lets you get the pointer.ForeignType::into_ptr
consumes/moves ownership, helpful for C functions that take over ownership.&self
parameter, most methods should be implemented on the borrowedXYZRef
type. This is because the ownedXYZ
type auto de-refs to it, enabling methods to work on both types.Clone
is supported, thenToOwned
is implemented by the macro, which is nice. This is how you go fromXYZRef
toXYZ
(viaXYZRef::to_owned
), i.e. how you convert from a borrowed reference to an owned instance.type CType
is implicitly expected to beSized
. Therefore (and I could be missing something), the type alias inbindgen
can't be explicitly used. For examplebindgen
gives ustype OGRSpatialReferenceH = *mut libc::c_void
, but inForeignType
implementation,type CType = OGRSpatialReferenceH
will cause errors. Instead it ends up needing to betype CType = libc::c_void
, which seems unfortunate when usingforeign-types
withbindgen
. Might be worth a question to the maintainer.Layer
) where the code can be made significantly more principled throughforeign_types
:Layer
was holding onto a&Defn
solely for the lifetime parameter tracking, which wasn't technically needed originally, and easier to eliminate withforeign_types
.OwnedLayer
can go away.owned: bool
fields that exist in some types because this is explicit in the type; e.g.Geometry::owned
.Opaque
and implementForeignTypeRef
. e.g.RasterBand
.CslStringList
) can be effectively managed by this library.RasterBand
are decoupled fromDataset
. Even thoughDataset
owns everyRasterBand
and defines its lifetime, you can mutate aRasterBand
without having a mutable reference toDataset
, which seems wrong. I think this is what we want to do: Underforeign_types
,Dataset::rasterband(&self, band_index: isize)
would returnResult<&mut Rasterband>
. Or maybe there's a new methodDataset::rasterband_mut(&mut self, band_index: isize)
. See Remaining Tasks above.