Skip to content
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

impl_scalar_property_reader! for usize? #8

Closed
michaelkirk opened this issue Feb 25, 2021 · 4 comments
Closed

impl_scalar_property_reader! for usize? #8

michaelkirk opened this issue Feb 25, 2021 · 4 comments

Comments

@michaelkirk
Copy link
Member

michaelkirk commented Feb 25, 2021

re: flatgeobuf/flatgeobuf#109 (comment)

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
Copy link
Member

pka commented Feb 25, 2021

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

@michaelkirk
Copy link
Member Author

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: serde-rs/serde#718

@pka
Copy link
Member

pka commented Feb 25, 2021

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.

@pka pka closed this as completed Feb 25, 2021
@michaelkirk
Copy link
Member Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants