Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems unfortunate to have behaviour of this that is dependent on the target, but since the underlying type requires it, why not
#[cfg]
or even just check at runtime if the platform considersUNIX_EPOCH
the minimal representable timestamp and just not bother with the add/sub on those targets?Would save on the input bytes and such.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The docs for
SystemTime
don't guarantee anything regarding what fits in aSystemTime
. So I would not know what to check for. MaybeUNIX_EPOCH
is the minimum. Maybe the year 0 is the minimum.Also, the maximum is interesting. It is not guaranteed that
SystemTime::UNIX_EPOCH + Duration::MAX
will fit. Therefore, it needs to be checked.I could determine the minimum and maximum by trail and error, save this result and use that to make sure we never generate a value that doesn't fit. But that isn't great code as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
UNIX_EPOCH.checked_sub(Duration::from_nanos(1)).is_none()
implies thatUNIX_EPOCH
is the min value representable. This could be checked once and saved in a static or somesuch.Unfortunately I've no good ideas for the maximum, so yeah, you would need the loop regardless. Feel free to ignore the comment until others chime in.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I understand, the minimum and maximum are platform-specific. I see three possible solutions: