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

Instant/SystemTime doc: add meaning to first line #30591

Merged
merged 2 commits into from
Jan 15, 2016

Conversation

SimonSapin
Copy link
Contributor

The first line (paragraph?) of a doc-comment is what rustdoc shows when listing items of a module.

What makes Instant and SystemTime different is important enough to be there. (Though feel free to bikeshed the wording.)

The first line (paragraph?) of a doc-comment is what rustdoc shows when listing items of a module.

What makes `Instant` and `SystemTime` different is important enough to be there. (Though feel free to bikeshed the wording.)
@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @aturon (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.

@brson
Copy link
Contributor

brson commented Dec 29, 2015

The focus on 'process' here may be confusing since the differences aren't explained in the main text in terms of processes.

@SimonSapin
Copy link
Contributor Author

Fair point, though I think the difference I’m trying to underline is implicit in the current main text. (Instant being opaque.) Instead of "the process", what do you think of "the life time of the program" (confusing with 'a) or "one run of the program"?

@brson
Copy link
Contributor

brson commented Dec 29, 2015

The point you are making about processes isn't clear to me regardless of the terminology - I don't draw the same implicit conclusion from reading the description.

The implications for processes seems to me a secondary attribute of the behavior of these types, not the main thing about them, suitable for the short description. If there are implications for how these types interact with processes I would expect that be explained more clearly in the text as an elaboration.

@aturon
Copy link
Member

aturon commented Jan 1, 2016

I agree with @brson. Perhaps for Instant we can emphasize it being opaque in the short description? I think if we did that, we could also leave the SystemTime short description as-is.

@SimonSapin
Copy link
Contributor Author

Ok, let’s forget the phrasing in this PR. What prompted it for me is looking at http://doc.rust-lang.org/nightly/std/time/ , where only the first paragraph of the doc of each item is shown. Instant and SystemTime seem similar yet there must be a reason they’re not the same. I know the difference and the reasons because I’ve read the relevant RFC, but no every reader of this page will have.

From these first paragraphs, I read that Instant is monotonically increasing, and SystemTime is for filesystem timestamps. While correct, I think some important points are missing here.

Instant being opaque is most important, but even the implications of that should be spelled out: it can not be in "persistent" storage or sent to a "remote" system. (I’m being deliberately hand-wavy about what that means exactly.) Comparing it to another instant to get a duration (or adding/substracting a duration to get another instant) is the only thing that can be done with it.

(Unless someone is tempted to parse format!("{:?}", instant), which maybe should be opaque as well?)

This means that SystemTime is probably the one to use for everything else. This is much more than timestamps in std::fs::Metadata.

@aturon
Copy link
Member

aturon commented Jan 6, 2016

cc @wycats -- I'm sure you have thoughts here.

@aturon
Copy link
Member

aturon commented Jan 6, 2016

@SimonSapin OK, everything you're saying makes sense to me. Spitballing:

  • Instant: A measurement of a monotonically increasing clock. Opaque and useful only with Duration.
  • SystemTime: A measurement from the system clock, useful for talking to external entities like the file system or other processes.

@SimonSapin
Copy link
Contributor Author

Looks good to me. Thanks!

@brson
Copy link
Contributor

brson commented Jan 11, 2016

I like the direction this is going now.

@SimonSapin
Copy link
Contributor Author

PR updated with @aturon’s wording.

@aturon
Copy link
Member

aturon commented Jan 15, 2016

@bors: r+ rollup

@bors
Copy link
Contributor

bors commented Jan 15, 2016

📌 Commit 8e2b4b2 has been approved by aturon

Manishearth added a commit to Manishearth/rust that referenced this pull request Jan 15, 2016
The first line (paragraph?) of a doc-comment is what rustdoc shows when listing items of a module.

What makes `Instant` and `SystemTime` different is important enough to be there. (Though feel free to bikeshed the wording.)
bors added a commit that referenced this pull request Jan 15, 2016
@bors bors merged commit 8e2b4b2 into rust-lang:master Jan 15, 2016
@SimonSapin SimonSapin deleted the patch-15 branch February 18, 2016 15:57
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.

5 participants