georust / geozero

Zero-Copy reading and writing of geospatial data.
Apache License 2.0
342 stars 35 forks source link

`impl_scalar_property_reader!` for usize? #8

Closed michaelkirk closed 3 years ago

michaelkirk commented 3 years ago

re: https://github.com/flatgeobuf/flatgeobuf/pull/109#issuecomment-785682610

let population: Option<usize> = feature.property("population");

It looks like there is no impl_scalar_property_reader! for usize.

In my specific use case, I realized it was inappropriate use of usize anyway, and switched to a u64, so everything is working for me.

Is usize something you want to support?

It seems somewhat fraught as a serializable type since it can have different size on different machines.

pka commented 3 years ago

I didn't think about it earlier, but I it shouldn't cause problems to support usize. Could be convenient sometimes.

michaelkirk commented 3 years ago

I trust that the code to support it could be written easily enough, but the question that arises is what happens when you serialize large values on platforms where usize is 64bit, and then deserialize on a platform where usize is 32bit?

protobuf doesn't support it: https://developers.google.com/protocol-buffers/docs/proto3#scalar flatbuffer doesn't support it: https://google.github.io/flatbuffers/md__schemas.html serde seemingly had, but then removed, support for it: https://github.com/serde-rs/serde/issues/718

pka commented 3 years ago

Thanks for your research! You convinced me that it's better not to support a usize conversion.

Using feature.property::<u32>("population") as usize is probably good enough.

michaelkirk commented 3 years ago

I guess this turned into a please-never-have-this-Feature Request. 😅