Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

impl Arbitrary for SystemTime #178

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

caspermeijn
Copy link
Contributor

No description provided.

fn arbitrary(u: &mut Unstructured<'a>) -> Result<Self> {
// Create a SystemTime by adding or subtracting a duration from epoch.
// The result is not guaranteed to fit. Keep trying until a good SystemTime if found.
loop {
Copy link
Member

@nagisa nagisa Mar 16, 2024

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 considers UNIX_EPOCH the minimal representable timestamp and just not bother with the add/sub on those targets?

Would save on the input bytes and such.

Copy link
Contributor Author

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 a SystemTime. So I would not know what to check for. Maybe UNIX_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.

Copy link
Member

@nagisa nagisa Mar 19, 2024

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 that UNIX_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.

Copy link
Contributor Author

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:

  1. Try to generate a value, check whether in range, if not, loop (current patch)
  2. Try to find min and max, save those values, then generate a value in between (thus moving the loop to the process of finding the min and max)
  3. Work with upstream to define the min and max as a constant (Duration has a min/max, so why shouldn't SystemTime)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants