Closed metasim closed 4 months ago
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 raster
bindings was pretty straight forward, the vector
and mdarray
bindings are complicated enough to require the original author of those to do the work, or explain how ownership/exclusive access should be handled to someone else to take on the work.
Remaining Tasks:
Dataset
(dependency https://github.com/georust/gdal/pull/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.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:
This 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.