Join GitHub today
GitHub is home to over 31 million developers working together to host and review code, manage projects, and build software together.
Sign upTime units #52556
Conversation
rust-highfive
assigned
Kimundi
Jul 20, 2018
This comment has been minimized.
This comment has been minimized.
|
r? @Kimundi (rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
label
Jul 20, 2018
This comment was marked as outdated.
This comment was marked as outdated.
|
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
This comment has been minimized.
This comment has been minimized.
|
Hm, what will be the best approach to solve the issue with |
kennytm
reviewed
Jul 20, 2018
| @@ -501,6 +524,15 @@ impl Mul<u32> for Duration { | |||
| } | |||
| } | |||
|
|
|||
| #[unstable(feature = "time_units", issue = "0")] | |||
| impl Mul<Duration> for u32 { | |||
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
newpavlov
Jul 20, 2018
•
Author
Contributor
Should I keep the attribute or use instead something like #[stable(feature = "time_units", since = "1.29.0")]?
This comment has been minimized.
This comment has been minimized.
kennytm
Jul 20, 2018
•
Member
@newpavlov The latter (with a different feature name, otherwise tidy will reject it).
This comment has been minimized.
This comment has been minimized.
newpavlov
Jul 20, 2018
•
Author
Contributor
Hm, what should I use for feature then? With time_units it can not be built.
UPD: Ah, haven't noticed your update. :)
| const H: Duration = Duration::from_secs(60*60); | ||
| /// 1 day `Duration` | ||
| #[unstable(feature = "time_units", issue = "0")] | ||
| const D: Duration = Duration::from_secs(24*60*60); |
This comment has been minimized.
This comment has been minimized.
kennytm
Jul 20, 2018
Member
Similar to #47097, I doubt we want to support hours and days.
Abbreviating minutes to M is also confusing (months? meters?). The SI standard short form MIN conflicts in meaning with "minimum". I would like to leave this out as well, keeping only S, MS, US and NS.
| @@ -30,6 +30,7 @@ use sys_common::FromInner; | |||
|
|
|||
| #[stable(feature = "time", since = "1.3.0")] | |||
| pub use core::time::Duration; | |||
| pub use core::time::{NS, US, MS, S, M, H, D}; | |||
This comment has been minimized.
This comment has been minimized.
kennytm
added
T-libs
relnotes
labels
Jul 20, 2018
This comment has been minimized.
This comment has been minimized.
|
Prior art: C++ spells the types out in full, https://en.cppreference.com/w/cpp/header/chrono#Convenience_Typedefs, though the literal suffixes abbreviate. The consts are a bit awkward since they encourage things like The types in C++ are often used as functions; what if these became function instead? The reading order is admittedly backwards, but |
This comment was marked as resolved.
This comment was marked as resolved.
|
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
This comment has been minimized.
This comment has been minimized.
|
For now I've removed minute, hour and day. Personally I don't have a strong need for them, so I guess it could be added later if someone will push for it. Although I'll note that #47097 the main issue was with dasy, not with hours and minutes, plus Go has hour and minutes as well (but not days). @scottmcm BTW if |
This comment was marked as resolved.
This comment was marked as resolved.
|
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
This comment was marked as resolved.
This comment was marked as resolved.
|
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
This comment was marked as resolved.
This comment was marked as resolved.
|
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
This comment was marked as resolved.
This comment was marked as resolved.
|
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
This comment was marked as resolved.
This comment was marked as resolved.
|
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
This comment has been minimized.
This comment has been minimized.
|
I think spelling out unit names would be a better idea (I don't have a strong opinion about functions vs constants), for the two reasons @scottmcm states (case sensitivity and name collisions) as well as the awkwardness of non-ASCII symbols (for microseconds, for instance). If we just pick the ASCII character that visually looks most similar to a particular non-ASCII character, it's going to exacerbate the other issues and can't necessarily be immediately guessed by a user, which isn't a problem if the full name is used. You can always alias when you import. |
This comment has been minimized.
This comment has been minimized.
|
@varkor @kennytm |
This comment has been minimized.
This comment has been minimized.
|
@newpavlov Those IMO |
This comment has been minimized.
This comment has been minimized.
|
Using I'm OK with either abbreviation or spelling out:
|
This comment has been minimized.
This comment has been minimized.
|
And what about As for |
This comment has been minimized.
This comment has been minimized.
|
I've returned |
This comment has been minimized.
This comment has been minimized.
The methods could be added on an extension trait in the right module which could be imported whenever these special methods are needed. I'm not saying this as someone for or against the proposal, I just wanted to help with a potential solution to this particular concern. I actually like both the constant and the method approach and would be totally fine with either. |
This comment has been minimized.
This comment has been minimized.
matthieu-m
commented
Jul 21, 2018
It would be possible to create a trait
Slightly more verbose (extraneous parentheses), but pretty idiomatic: traits have been used to provide method extensions in Rust forever ( |
This comment has been minimized.
This comment has been minimized.
|
@matthieu-m |
This comment has been minimized.
This comment has been minimized.
I quite like that, actually. Especially because then you import the one trait and it doesn't pollute the namespace the way that importing a bunch of constants does. And fixes the casing issue. |
This comment has been minimized.
This comment has been minimized.
HeroicKatora
commented
Jul 21, 2018
|
@matthieu-m I'm not sure about long or short form. I slightly favor the long form similar to the current interfaces (secs, millis, micros, nanos, etc.). In no case would I like a trait with cryptic names such as @newpavlov In my opinion, traits and being able to introduce new methods provide superior functionality and consistency compared to custom literals. In particular, the tradeoff of complicating the language when we could solve this with existing features does not sound worth here. It is also a little unclear to me what that would specifically accomplish here. Custom literals should give you power to reinterpret the literal in the code, it feels wrong to also use them if one simply wants to convert a conventionally parsed primitive into another type. Keep in mind, C++ has to resort to this style only because it does not have methods on primitive types. |
This comment has been minimized.
This comment has been minimized.
flexera-cnixon
commented
Jul 22, 2018
|
It seems there are enough opinions on this PR that it could make sense for this to be an RFC? The embedded-hal ecosystem is also thinking about things in this area rust-embedded/embedded-hal#59 |
This comment has been minimized.
This comment has been minimized.
|
@HeroicKatora sleep(1s + 100ms)
sleep(1.s() + 100.ms())The latter variant is more noisy, more surprising for beginners (what? is it a method on primitive integer?), more annoying to write and read, while the former takes minimum amount of characters and explicitly shows that you are not dealing with primitive integers, but with something else. Also if we take other units into consideration (which can be implemented by third-party crates, see e.g. |
This comment has been minimized.
This comment has been minimized.
HeroicKatora
commented
Jul 22, 2018
|
@newpavlov You mean noise as in there are three extra characters per suffix? I think I see where you are getting at. But the alternative is additional complexity in language, syntax and all the way down to the grammar. And I'd argue that the parts which you qualified as noise actually help process the code mentally in many cases, especially since it does not necessitate teaching and learning a new kind of rule but simply generalizes being able to call methods on values. In any case, it now appears that the original proposal does not seem like the best solution to either of us. So I would follow @flexera-cnixon by opening an RFC. I would also extend on my comment above about advantages and disadvantages for literal suffixes there. |
This comment has been minimized.
This comment has been minimized.
|
I've published pre-RFC for custom literals on internals forum. Let's continue discussion about them vs trait based approach there. |
This comment has been minimized.
This comment has been minimized.
|
I agree with @flexera-cnixon; it seems there's enough of a design space here that this PR should be closed in favour of a discussion on IRLO and/or a full RFC on that repo. |
This comment has been minimized.
This comment has been minimized.
I agree with this. We recently stabilized |
This comment has been minimized.
This comment has been minimized.
|
@scottmcm @WiSaGaN |
This comment has been minimized.
This comment has been minimized.
Why would this be valuable over putting things in a crate? This feature needs no special access to duration internals, nor does it need internal-only features (like intrinsics or similar), so why not just do (This isn't a method on a prelude trait, so there's no future-conflict hazard with a crate import.) |
This comment has been minimized.
This comment has been minimized.
|
You are right, I guess it can be in a separate crate. But convenience |
This comment has been minimized.
This comment has been minimized.
|
@newpavlov Scalar multiplication of durations definitely sounds useful and separable. At least for myself, I've certainly wanted things like Oh, |
This comment has been minimized.
This comment has been minimized.
I am not sure if I understood you, we already have |
This comment has been minimized.
This comment has been minimized.
Is this something not yet implemented or something we shouldn't do? If it is the former, then should we fix that instead?
|
WiSaGaN
referenced this pull request
Jul 25, 2018
Closed
associated constants can't be `use`d #24915
This comment has been minimized.
This comment has been minimized.
FWIW, I'd be in support of an RFC to allow |
This comment has been minimized.
This comment has been minimized.
|
If namespace pollution is the only problem we could introduce |
This comment has been minimized.
This comment has been minimized.
This was actually discussed in #24915 (comment) where a deprecation of Overall, I feel it would be beneficial to discuss the general treatment of introducing
|
This comment has been minimized.
This comment has been minimized.
|
BTW what do you think about adding division and multiplication by |
This comment has been minimized.
This comment has been minimized.
|
I am closing this PR and will create a separate one which will contain only additional Also I've published |
newpavlov commentedJul 20, 2018
•
edited
This PR adds
NANOSECOND,MICROSECOND,MILLISECOND,SECONDconstants to time module for bothstdandcore. Instead ofsleep(Duration::from_millis(10))it will allow us to write:As was discussed in #51610 adding constants for common durations will make creation of
Durationsignificantly more ergonomic. This PR is a draft which can be changed upon discussion. Small explanation of the choice made in this PR:NANOSECONDand notNS? While I think the latter is usually more ergonomic, abbreviation confuses error messages and can be ambiguous in some situations. Full names can be shortened via aliasing as was shown in the example earlier. Additionally this naming will be similar to the other languages (in particular to Go and C++).time::units? I think that a separate module will be redundant, currentlytimemodule has only one constantUNIX_EPOCH, thus they will not be lost in the documentation, and it will help with teaching existence of those constants.Mul<Duration> for u32? This will allow us to writethread::sleep(10*M + 20*S)instead of less pleasantthread::sleep(M*10 + S*20). Ideally I would like to have a way to describe commutative property of multiplication in this case to the type system, but oh well...MINUTE,HOURandDAY? While as units accepted for use with the SI they are strictly defined in number of seconds, there is caveats to those units if UTC is considered, e.g. hour can be 3,599–3,601 seconds long, depending on conditions. Thus to avoid confusion some argue its probably better to be conservative and not include them. (see #47097 for more context) Though we probably could use prefixes likeSI_orSTD_for those units to highlight the difference from UTC units. UPD: they are included back. Final decision can be made on feature stabilization.UPD: PR was updated based on replies.
UPD2: It looks like the prevalent opinion is that custom suffixes will be a better solution, i.e. an ability to write something like this:
So even if this feature will get merged, it will probably will not be stabilized and removed at some point.