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

Rollup of 6 pull requests #115303

Merged
merged 15 commits into from
Aug 28, 2023
Merged

Rollup of 6 pull requests #115303

merged 15 commits into from
Aug 28, 2023

Conversation

matthiaskrgr
Copy link
Member

Successful merges:

r? @ghost
@rustbot modify labels: rollup

Create a similar rollup

ijackson and others added 15 commits March 27, 2023 12:32
I edited one of these and looked at the formatted docs for the other.
This confused me for a while; I suspected a build system bug.

I don't see an easy and neat way to unify these.  So let's just
document it instead.
 * Make the description primary, not the definition in terms of time_t
 * Remove the list of Internet protocols

As per
  rust-lang#109660 (review)
Document that SystemTime does not count leap seconds

Fixes rust-lang#77994

This may not be entirely uncontroversial.  I know that `@Ekleog` is going to disagree.  However, in support of this docs change:

 This documents the current behaviour.  The alternative would be to plan to *change* the behaviour.

There are many programs which need to get a POSIX time (a `time_t`).  Right now, `duration_since(UNIX_EPOCH)` is the only facility in std that does that.  So, that is what programs use.  Changing the behaviour would  break[1] all of those programs.  We would need to define a new API that can be used to get a POSIX time, and get everyone to use it.  This seems highly unpalatable.

And, even if we wanted to do that, time with leap seconds is a lot less easy to work with.  We would need to arrange to have a leap seconds table available to `std` somehow, and make sure that it was kept up to date.  Currently we don't offer to do that for timezone data, which has similar needs.  There are other complications.  So it seems it would be awkwarrd to *implement* a facility that provides time including leap seconds, and the resulting value would be hard for applications to work with.

Therefore, I think it's clear that we don't want to plan to ever change `SystemTime`.  We should plan to keep it the way it is.  Providing TAI (for example) should be left to external crates, or additional APIs we may add in the future.

For more discussion see rust-lang#77994 and in particular `@fanf2's` rust-lang#77994 (comment)

[1]  Of course, by "break" we really only mean *future* breakage in the case where there is, in fact, ever another leap second.  There may well not be: they are in the process of being abolished (although this is of course being contested).  But if we decide that `SystemTime::now().duraton_since(UNIX_EPOCH)` counts leap seconds, it would start to return `Durations`s that are 27s different to the current answers.   That's clearly unacceptable.  And we can hardly change `UNIX_EPOCH` by 27s.
Fix implementation of `Duration::checked_div`

I ran across this while running some sanity checks on `time`. Quickcheck immediately found a bug, and as I'd modified the code from `std` I knew there was a bug here as well.

tl;dr this code fails ([playground](https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=1189a3efcdfc192c27d6d87815359353))

```rust
use std::time::Duration;

fn main() {
    assert_eq!(
        Duration::new(1, 1).checked_div(7),
        Some(Duration::new(0, 142_857_143)),
    );
}
```

The existing code determines that 1/7 = 0 (seconds), 1/7 = 0 (nanoseconds), 1 billion / 7 = 142,857,142 (extra nanoseconds). The billion comes from multiplying the remainder of the seconds (1) by the number of nanoseconds in a second. However, **this wrongly ignores any remaining nanoseconds**. This PR takes that into consideration, adds a test, and also changes the roundabout way of calculating the remainder into directly computing it.

Note: This is _not_ a rounding error. This result divides evenly.

`@rustbot` label +A-time +C-bug +S-waiting-on-reviewer +T-libs
std/tests: disable ancillary tests on freebsd since the feature itsel…

…f is.
…=calebcartwright

style-guide: Add guidance for defining formatting for specific macros
…illaumeGomez

tell people what to do when removing an error code

Currently tidy and CI send developers on a wild goose chase:
- you edit the code
- CI/tidy tells you that an error code is gone, so you remove it from the list
- CI/tidy tells you that the markdown file is stale, so you remove that as well
- CI (but not tidy) tells you not to remove an error description and copy what E0001 does

Let's be nice to people and directly tell them what to do rather than making them follow misleading breadcrumbs.

r? ``@GuillaumeGomez``
…trace, r=Amanieu

avoid triple-backtrace due to panic-during-cleanup

Supersedes rust-lang#115020
Cc rust-lang#114954
r? ``@Amanieu``
@rustbot rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc O-unix Operating system: Unix-like S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. T-style Relevant to the style team, which will review and decide on the PR/issue. rollup A PR which is a rollup labels Aug 28, 2023
@matthiaskrgr
Copy link
Member Author

@bors r+ rollup=never p=6

@bors
Copy link
Contributor

bors commented Aug 28, 2023

📌 Commit 32053f7 has been approved by matthiaskrgr

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 28, 2023
@bors
Copy link
Contributor

bors commented Aug 28, 2023

⌛ Testing commit 32053f7 with merge 7e02fd8...

@bors
Copy link
Contributor

bors commented Aug 28, 2023

☀️ Test successful - checks-actions
Approved by: matthiaskrgr
Pushing 7e02fd8 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 28, 2023
@bors bors merged commit 7e02fd8 into rust-lang:master Aug 28, 2023
@rustbot rustbot added this to the 1.74.0 milestone Aug 28, 2023
@rust-timer
Copy link
Collaborator

📌 Perf builds for each rolled up PR:

PR# Message Perf Build Sha
#109660 Document that SystemTime does not count leap seconds 4a89e089d06670fd48d12565b3c89683806f3e98 (link)
#114238 Fix implementation of Duration::checked_div 7b889da8ebbf131f351a57b2527b8f38d66fd8af (link)
#114512 std/tests: disable ancillary tests on freebsd since the fea… 543ae82e4a53f7b45104f23a4ade70f9295702a4 (link)
#114919 style-guide: Add guidance for defining formatting for speci… 5f50b3ef8b8c2a7c87b14d0c8a33cb6b0a52c630 (link)
#115278 tell people what to do when removing an error code bb1602670d36e4eb0180a51d6ff1faa3b64f1269 (link)
#115280 avoid triple-backtrace due to panic-during-cleanup fe48da694315ff0e09b345143ce5f29803b0b637 (link)

previous master: 41cb42a370

In the case of a perf regression, run the following command for each PR you suspect might be the cause: @rust-timer build $SHA

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (7e02fd8): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
2.9% [1.2%, 4.4%] 5
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 631.418s -> 631.432s (0.00%)
Artifact size: 316.30 MiB -> 316.28 MiB (-0.01%)

@matthiaskrgr matthiaskrgr deleted the rollup-iohs8a5 branch March 16, 2024 18:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-testsuite Area: The testsuite used to check the correctness of rustc merged-by-bors This PR was explicitly merged by bors. O-unix Operating system: Unix-like rollup A PR which is a rollup S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. T-style Relevant to the style team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants