Implement date --resolution
#6272
Conversation
|
GNU testsuite comparison: |
|
GNU testsuite comparison: |
There was a problem hiding this comment.
I think your overall approach "try to use libc::clock_getres, try to handle any errors sensibly" is a good approach, and exactly the right function for this job. Great! :D
See below for many changes.
Also, please write tests, ideally so that CI can test both the CLOCK_REALTIME and CLOCK_MONOTONIC paths. I'm not entirely convinced whether it is a good idea to even query CLOCK_MONOTONIC, because if CLOCK_REALTIME is not available, then the system is either extremely non-conformant or about to kernel panic anyway. And CLOCK_MONOTONIC measures something completely different. When you write tests for this, I'm sure you will stumble upon either a good argument why this is a good idea to keep in, or you will find that all systems support CLOCK_REALTIME anyway.
| @@ -1,17 +0,0 @@ | |||
| repos: | |||
There was a problem hiding this comment.
Please don't delete this unrelated file, other people need it :D
| let resolution_secs: f32 = | ||
| res_ts.tv_sec as f32 + res_ts.tv_nsec as f32 / RESOLUTION_DIVISOR; | ||
| resolution_secs |
There was a problem hiding this comment.
| let resolution_secs: f32 = | |
| res_ts.tv_sec as f32 + res_ts.tv_nsec as f32 / RESOLUTION_DIVISOR; | |
| resolution_secs | |
| res_ts.tv_sec as f32 + res_ts.tv_nsec as f32 / RESOLUTION_DIVISOR |
| @@ -103,6 +107,75 @@ enum Iso8601Format { | |||
| Ns, | |||
| } | |||
|
|
|||
| // Enum for system clock resolution | |||
| enum TimeResolution {} | |||
There was a problem hiding this comment.
I don't understand why this needs to be a type.
| let mut resolution_final: String = String::new(); | ||
| resolution_final.push_str("0."); | ||
| let mut i: u8 = 0; | ||
| while i <= resolution_length { | ||
| resolution_final.push('0'); | ||
| i += 1; | ||
| } | ||
| resolution_final.push('1'); | ||
|
|
||
| resolution_final.parse::<f32>().unwrap_or(0.1) |
There was a problem hiding this comment.
| let mut resolution_final: String = String::new(); | |
| resolution_final.push_str("0."); | |
| let mut i: u8 = 0; | |
| while i <= resolution_length { | |
| resolution_final.push('0'); | |
| i += 1; | |
| } | |
| resolution_final.push('1'); | |
| resolution_final.parse::<f32>().unwrap_or(0.1) | |
| (0.1).powi(resolution_length) |
Also, I'm unhappy that this "guessing" has to happen at all. For example, on a system with a 1-nanosecond resolution it will claim a worse resolution once every 9 tries (10% + 1% + 0.1% + …), that's the perfect breeding ground for Heisenbugs.
If this guessing approach is kept, please add tests to it. (See below.)
| }; | ||
|
|
||
| resolution |
There was a problem hiding this comment.
This can be simplified.
Example:
fn foo() {
let bar = match { /* ... */ };
bar
}
… can be turned into:
fn foo() {
match { /* ... */ } // No semi-colon
}
| } | ||
|
|
||
| fn clock_type(ts: &mut timespec) -> Option<i32> { | ||
| if unsafe { libc::clock_gettime(CLOCK_REALTIME, &mut *ts) } == 0 { |
There was a problem hiding this comment.
Why is the function unsafe, and why is it safe to use in this particular case? Please write the second answer in a comment, for both function calls.
Why &mut *ts instead of just ts? Rust should automatically coerce a mut-ref to a mut-pointer, and testing this in the playground, this should work.
| } else if unsafe { libc::clock_gettime(CLOCK_MONOTONIC, &mut *ts) } == 0 { | ||
| Some(CLOCK_MONOTONIC) | ||
| } else { | ||
| None |
There was a problem hiding this comment.
When can this ever happen? If the system is really borked to this degree, then we probably shouldn't guess anyway.
Oh, and this is a bug: If this case ever happens, then ts probably contains garbage data, and must not be used to estimate anything. I suggest just returning "resolution is 1 second" in that scenario.
| .long(OPT_RESOLUTION) | ||
| .value_name("RESOLUTION") | ||
| .help("output the available resolution of timestamps") | ||
| .action(ArgAction::SetTrue), |
There was a problem hiding this comment.
When you implement it, please make sure that you set overrides_with, so that date --resolution --resolution also works :)
Please implement and also test this.
| } | ||
| Some(CLOCK_MONOTONIC) => { | ||
| let mut res_ts: timespec = ts; | ||
| unsafe { libc::clock_getres(CLOCK_MONOTONIC, &mut res_ts) }; |
There was a problem hiding this comment.
clock_getres can fail, and the return value must be checked: https://docs.rs/libc/latest/libc/fn.clock_getres.html
And because it must be checked anyway, I don't understand why this code calls SystemTime::now(); and then also clock_gettime.
|
ping ? :) |
Implementing the
--resolutionflag for date