Make duration_since documentation more clear

[abrausch]

No description provided.

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @cramertj (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[pickfire]

Other than the last suggestion. Everything looks good to me.

[pickfire]

Looks good to me.

[jyn514]

Hmm, I know @pickfire suggested checked_add but I don't think it really gets the point of the example across - this will never be an Err because the checked_add will mean it will always be later than before.

I think I'd prefer no_run and having the sleep from before (plus add use std::thread at the top):

Suggested change

	/// let new_sys_time = sys_time.checked_add(Duration::new(1, 0)).unwrap();
	/// let difference = new_sys_time.duration_since(sys_time)
	/// .expect("Clock may have gone backwards");
	/// thread::sleep(Duration::from_secs(1));
	/// let new_sys_time = SystemTime::now();
	/// let difference = new_sys_time.duration_since(sys_time)
	/// .expect("Clock may have gone backwards");

[pickfire]

@jyn514 I know the previous example is better but it cannot be run with no_run, if any I would prefer to see a mock instead of both of the solutions we have right now. It can both tell the sleep as well as teaching the readers how to mock time in tests, we don't have any of it written right now.

[jyn514]

Right, but I think having no_run is better than having an example that doesn't make sense.
Another alternative is to remove the sleep altogether, but then it really needs to be no_run because otherwise it will sometimes fail.

[pickfire]

Right, but I think having no_run is better than having an example that doesn't make sense.

My suggestion for this is to add a comment that saying it is used to replace sleep.

Another alternative is to remove the sleep altogether, but then it really needs to be no_run because otherwise it will sometimes fail.

If we do that then this patch does not do anything at all. So it remains the same.

[jyn514]

No, the previous code used the same duration for the comparison, which would never fail because it would never go backwards. This now uses a new duration, which might fail because system time is not monotonic.

[pickfire]

This now uses a new duration, which might fail because system time is not monotonic.

Oh, I forgot about this. Yeah, this is why time may go backwards.

[jyn514]

Glad we're in agreement 😆

@abrausch do you mind updating this to use no_run to make it more clear why this is a potential footgun?

Suggested change

	/// let new_sys_time = sys_time.checked_add(Duration::new(1, 0)).unwrap();
	/// let difference = new_sys_time.duration_since(sys_time)
	/// .expect("Clock may have gone backwards");
	/// let new_sys_time = SystemTime::now();
	/// let difference = new_sys_time.duration_since(sys_time)
	/// .expect("Clock may have gone backwards");

[jyn514]

@bors r+ rollup

Thanks for your PR and for your patience!

[bors]

📌 Commit 98231bf has been approved by jyn514