gdal icon indicating copy to clipboard operation
gdal copied to clipboard

Replace `OGRwkbGeometryType::Type` (a.k.a. c_uint) by `enum GeometryType`

Open ttencate opened this issue 2 years ago • 6 comments

Previously discussed on #250. The question was how extensible it should be, which I've briefly looked into.

  1. It looks like the last addition happened six years ago so it's definitely not changing frequently.
  2. The gdal_sys crate needs updating with every new release anyway due to the generated bindings, so adding new constants to the Rust enum manually should not be too much trouble. Or after the fact if nobody has a need for them yet.
  3. We can use #[non_exhaustive] so that client code is forced to reckon with new additions to the enum.
  4. What to do if an unsupported value is returned by a GDAL/OGR API? Assuming the Rust enum has been kept up to date, this can only mean that we are running with a newer version than we were compiled with. Is this even a supported scenario? I don't know if GDAL/OGR gives any ABI stability guarantees.
  5. What to do if a value of the Rust enum is not supported by the GDAL/OGR version we compiled with? Can we conditionally enable enum variants depending on the compile-time GDAL/OGR version?

Note that the issues are mostly hypothetical at the moment; OGRwkbGeometryType has the same values in all of the supported GDAL/OGR versions.

ttencate avatar Feb 01 '22 12:02 ttencate

I'm in favor of wrapping these constants with enums.

  • #[non_exhaustive] isn't worse than fiddling with these numbers.
    • We could use wkbUnknown as long as we don't know it.
  • You are right that new version-bindings could only lead to new values. However, we would need some mechanism to catch new versions. Last time, it took us some while for new bindings. Moreover, can we have a mechanism that checks for all values being covered?
  • I guess we can conditionally enable variants. In the worst case, we can conditionally switch between enums.

ChristianBeilschmidt avatar Feb 02 '22 05:02 ChristianBeilschmidt

I honestly don't know what would be the best solution here, but I want to understand why you want this change.

  • is it because it's annoying to import gdal-sys and type those OGRwkb prefixes?
  • is it so matching on the geometry gets a little nicer?
  • or because an enum would be more strongly-typed, so you can't pass a wrong value by mistake?
  • or maybe something else?

lnicola avatar Feb 02 '22 06:02 lnicola

All of the above. Plus the ability to implement useful traits like Debug and Display on the type (which would have made #250 more idiomatic).

ttencate avatar Feb 02 '22 07:02 ttencate

What if we wrapped them in structs defined gdal? Something like:

#[derive(PartialEq, Eq)]
struct GeometryType(u32);

impl GeometryType {
    pub const Unknown: GeometryType = GeometryType(0);
    pub const Point: GeometryType = GeometryType(1);
    pub const LineString: GeometryType = GeometryType(2);
}
  • you don't have to import gdal-sys and it has a nicer name
  • you can use match (and must exhaustively do so)
  • it's strongly-typed
  • you can implement Debug and Display

lnicola avatar Feb 02 '22 07:02 lnicola

In what way is that better than an enum though? If it's purely to allow unknown values through, we can also do that with an enum.

ttencate avatar Feb 02 '22 07:02 ttencate

You're right, it's not much better than an enum with an Unknown(u32) variant. I guess it would avoid some mapping code from the enum to integers and back.

lnicola avatar Feb 02 '22 07:02 lnicola