datrs / random-access-storage

Abstract interface to implement random-access instances.
Apache License 2.0
13 stars 7 forks source link

Use u64 for storage length #6

Closed khernyo closed 5 years ago

khernyo commented 6 years ago

usize is 32 bits on a 32 bit system, so the storage would be limited to 4GB on such a system.

u64 should be enough for now. (std::fs::Metadata::len() is u64, so in theory it's still not enough to handle a huge amount of gigantic files).

To be exact, all uses of usize need changing here: https://github.com/datrs/random-access-storage/blob/50b947e688ef3ea5addd81991386ae389a66c1f6/src/lib.rs. And then the breakages in the other datrs modules need fixing.

yoshuawuyts commented 6 years ago

I'm so-so about this change. Mainly because all indexing methods in Rust's stdlib use usize and casting all over the code would be rather annoying. I feel sticking to usize gives us easier code (:

khernyo commented 6 years ago

Yes, the code is cleaner but I think the plan is to support wasm where usize is 32 bits. I think it would look ugly if the js implementation of dat had support for large files and datrs did not. :)

Anyway, there are more pressing issues, that's for sure.

ghost commented 5 years ago

casting is annoying but I'm very soon going to run into this issue for a database I'm building which deals with offsets much bigger than 4GB, which won't work on systems where usize is 32 bits.

yoshuawuyts commented 5 years ago

Open to change it to u64!

jackjennings commented 5 years ago

I'm willing to drive this change amongst the different random-access-* components of datrs. I assume this will also mean releasing a v3.0.0 major version?

khernyo commented 5 years ago

Fixed in #17