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

Duration doc #38346

Merged
merged 1 commit into from
Dec 21, 2016
Merged

Duration doc #38346

merged 1 commit into from
Dec 21, 2016

Conversation

GuillaumeGomez
Copy link
Member

@GuillaumeGomez GuillaumeGomez force-pushed the duration_doc branch 2 times, most recently from 22088dc to 57353e2 Compare December 13, 2016 14:55
Copy link
Member

@frewsxcv frewsxcv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

r=me unless you want to address my comment

/// ```
/// use std::time::Duration;
///
/// let five_seconds = Duration::from_millis(5010);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not exactly five seconds so the variable binding name is slightly misleading. Could also just call it something generic like duration. Not a super big deal.

@@ -136,9 +184,11 @@ impl Duration {
}
}

/// Checked duration subtraction. Computes `self + other`, returning `None`
/// Checked `Duration` subtraction. Computes `self - other`, returning [`None`]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Glad you fixed the +- error here :)

/// Durations implement many common traits, including `Add`, `Sub`, and other
/// ops traits. Currently a duration may only be inspected for its number of
/// `Duration`s implement many common traits, including [`Add`], [`Sub`], and other
/// [`ops`] traits. Currently a `Duration` may only be inspected for its number of
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This last sentence is not true anymore – you may now do arithmetic on duration as well.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll remove it then.

@@ -207,9 +259,11 @@ impl Duration {
}
}

/// Checked duration division. Computes `self / other`, returning `None`
/// Checked `Duration` division. Computes `self / other`, returning [`None`]
/// if `other == 0` or the operation results in underflow or overflow.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Drive-by fix: division cannot underflow or overflow in other != Duraton::from_secs(0) as having a negative or floaty duration is not possible.

/// Checked duration multiplication. Computes `self * other`, returning
/// `None` if underflow or overflow occurred.
/// Checked `Duration` multiplication. Computes `self * other`, returning
/// [`None`] if underflow or overflow occurred.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Drive-by fix: multiplication cannot underflow as negative durations are impossible.

@GuillaumeGomez
Copy link
Member Author

Updated.

@frewsxcv
Copy link
Member

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Dec 14, 2016

📌 Commit 60fbe7a has been approved by frewsxcv

GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Dec 15, 2016
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Dec 16, 2016
@bors
Copy link
Contributor

bors commented Dec 16, 2016

⌛ Testing commit 60fbe7a with merge 47d4a02...

@bors
Copy link
Contributor

bors commented Dec 16, 2016

💔 Test failed - auto-mac-64-opt-rustbuild

@GuillaumeGomez
Copy link
Member Author

@bors: retry

@bors
Copy link
Contributor

bors commented Dec 16, 2016

⌛ Testing commit 60fbe7a with merge a6a4315...

@bors
Copy link
Contributor

bors commented Dec 16, 2016

💔 Test failed - auto-mac-64-opt-rustbuild

@alexcrichton
Copy link
Member

alexcrichton commented Dec 16, 2016 via email

GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Dec 18, 2016
@sanxiyn
Copy link
Member

sanxiyn commented Dec 19, 2016

@bors retry

@alexcrichton
Copy link
Member

@bors: retry

alexcrichton added a commit to alexcrichton/rust that referenced this pull request Dec 20, 2016
bors added a commit that referenced this pull request Dec 20, 2016
@bors bors merged commit 60fbe7a into rust-lang:master Dec 21, 2016
@GuillaumeGomez GuillaumeGomez deleted the duration_doc branch December 21, 2016 16:37
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.

6 participants