search

alexcrichton / filetime

Accessing file timestamps in a platform-agnostic fashion in Rust
Apache License 2.0
122 stars 56 forks source link

Add function to create FileTime from SystemTime #22

Closed Ortham closed 6 years ago

Ortham commented 6 years ago

My primary motivation for this is to be able to handle pre-Unix-epoch timestamps, and having a from_system_time() is also useful as I'm currently converting to and from Unix epoch to get from SystemTime to FileTime.

As was suggested in #6, I've avoided breaking existing API functions by casting between signed and unsigned integers, though this means that nonsense will be returned by the seconds(), seconds_relative_to_1970() and nanoseconds() functions on Unixy systems when FileTime is pre-Unix-epoch. I think breakage would be better as it is a 0.1.x library and to/from Unix timestamp functions should really be able to handle the case where they're pre-Unix-epoch, but I thought I'd make minimal changes and see what you thought.

I'm not sure if the handling of mtime_nsec() values is correct, the docs don't suggest it's a fractional second value, but I've preserved the existing behaviour, modulo unsigned -> signed cast.

I've tested the changes on Windows and Linux, I'm not sure about other OSes but I tried to update them as I did for Linux.

alexcrichton commented 6 years ago

Thanks for the PR! This sounds like great functionality to have and I think I was also a little too eager to use unsigned integers for sure! I'm actually totally fine releasing 0.2 for this library with a breaking change, so wanna go ahead and change methods like seconds_relative_to_1970?

Ortham commented 6 years ago

I'm actually totally fine releasing 0.2 for this library with a breaking change, so wanna go ahead and change methods like seconds_relative_to_1970?

Sure. Would you be open to renaming the _1970 methods? As it is I think they're ambiguous ("when in 1970"?), I think from_unix_timestamp(i64, i32) and unix_timestamp_seconds() (or just unix_timestamp() given there is no subsecond precision in Unix timekeeping) makes it clearer that they're relative to the start of the Unix epoch. Also because the "since" in from_seconds_since_1970() seems to imply a non-negative value to me.

Ortham commented 6 years ago

I think casting away the sign when reading file metadata may cause overflow problems when dealing with negative nanosecond values, which seems to be possible in some OSs and not others, but the standard library's MetadataExt doesn't expose such variation (not that it needs to, all positive values will fit in an i32, let alone i64). Whatever the reality, I'm considering that out-of-scope for this PR, but I thought it's worth mentioning.

Ortham commented 6 years ago

@alexcrichton I think I'm done, could you review?

alexcrichton commented 6 years ago

Looks fantastic to me, thanks!

Ortham commented 6 years ago

Thanks for the merge, any chance of publishing a release to crates.io? AFAIK I can't publish my dependent crate unless all its dependencies are already published. No rush though, I won't be in a position to do so until next week anyway.

alexcrichton commented 6 years ago

Sure thing, done!