Constify SystemTime methods
#144519
Constify SystemTime methods
#144519
Conversation
|
Some changes occurred in src/tools/clippy cc @rust-lang/clippy |
|
(This does not, in fact, include changes to clippy. That's from the other PR and can be safely ignored.) |
This comment has been minimized.
This comment has been minimized.
|
I'm mildly surprised that this is even possible at compile time, but I guess it makes sense given the internals. My concern is that this would (if/when stabilized) provide a guarantee that all future targets having But if the team is fine with this, I'm not going to get in the way. The final commit LGTM once CI passes and it's unblocked. |
|
My main thought process is that we've effectively guaranteed that all targets have the ability to represent this due to the presence of We even have a direct example of this for Windows, documented here: the epoch for Windows' I also just find it very doubtful that any target would have a concept of "system time" where the units are not defined by the spec. The only theoretical target we could support now which would not fall under this system is one where:
Imagine such a target where, for example, the "current time" system call had configurable units: depending on the system configuration, such a system call would return either milliseconds or seconds since the epoch. That just sounds like an absolute hell to deal with when it comes to figuring out relative times it makes a lot more sense for there to just be different system calls for different units. Plus, what happens if the configuration changes while the system is running? Like, again, it's not strictly impossible to happen, but it feels like the inability to support this kind of functionality is just the target's problem, not ours. To me, it's equivalent to a target just not having any clock functionality, which is much more likely than this, despite that also being unlikely. (Unrelated to that, I will also definitely fix the compilation errors before this is unblocked. I definitely missed a couple feature flags & const attributes.) |
This comment was marked as resolved.
This comment was marked as resolved.
0019f4b to
784753f
Compare
|
After much pain and installing dependencies, I managed to replicate what should ensure that all the builds succeed. Each module that was modified appears to have been covered: With the target list being:
Xous seems to have some issues with the default build command due to a lack of Also to clarify: these will all be caught by the tier-2 build checks from bors, but I wanted to run them myself without clogging the bors queue. |
f42d938 to
e1320af
Compare
|
@rustbot author Requisite PR merged; will rebase and mark as ready. |
|
Reminder, once the PR becomes ready for a review, use |
e1320af to
15a79a3
Compare
|
@rustbot ready Local tier-2 build test still worked, gonna assume CI will pass as well. If it doesn't then I can fix it later. |
|
@rustbot author |
|
@rustbot blocked (again) |
|
@bors delegate+ |
|
✌️ @clarfonthey, you can now approve this pull request! If @jhpratt told you to " |
15a79a3 to
c839bd2
Compare
|
☔ The latest upstream changes (presumably #145300) made this pull request unmergeable. Please resolve the merge conflicts. |
c839bd2 to
7ce620d
Compare
|
Just to avoid bors thinking you're approving you're own PR @bors r- r+ |
|
Thank you 😓 |
…hpratt Constify `SystemTime` methods This is separated out from rust-lang#143949 due to the fact that it touches nontrivial system code. While the same arithmetic methods on `Instant` could be made const, since that type explicitly cannot be constructed at const-time, we don't bother. However, due to the fact that `SystemTime::UNIX_EPOCH` exists, it can be useful to create other anchors at other points in time, and thus these methods should be possible to use in const context. > Side comment: I would honestly like to just move every single implementation which is a thin wrapper over `Duration` or some integer clock into their own module which is tested on all targets, so that we don't have to worry about code for lower-tier targets not being tested. However, the comments in `std::sys_common` pointing out the desire to move things to `std::sys::common` confused me, particularly due to the fact that `std::sys::common` is taken from the hellish asterisk-import of `std::sys::pal::common` and others. > > I think that, for trivial types like this, we should just have a platform-independent module that can be tested on all platforms, to both ensure that the code is properly tested regardless of whether higher-tier platforms needed, and to avoid the extreme mental gymnastics required to determine where in the code it lies. > > However, since I'm not on any of the teams maintaining this code and am not involved, I'll settle for just copy-pasting like everyone else has been doing. I just want to express how I'm not happy about that. **Reviewer note: please only pay attention to the last commit of this PR, until its blocker is merged.** Since this depends on the previous PR: `@rustbot` blocked
This is separated out from #143949 due to the fact that it touches nontrivial system code. While the same arithmetic methods on
Instantcould be made const, since that type explicitly cannot be constructed at const-time, we don't bother. However, due to the fact thatSystemTime::UNIX_EPOCHexists, it can be useful to create other anchors at other points in time, and thus these methods should be possible to use in const context.Reviewer note: please only pay attention to the last commit of this PR, until its blocker is merged.
Since this depends on the previous PR:
@rustbot blocked