-
Notifications
You must be signed in to change notification settings - Fork 527
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
Support i128 and extend limits for TimeDelta in chrono 0.5 #933
Comments
Thanks for your comment @doomlaur - there is already some work underway that I think would solve the issues you are running into here. Specifically, we are aiming to remove |
Hello @esheppa, I'm happy to hear that! I think switching to
Of course, the best thing to do at this point would be to improve Some further ideas:
I think not having support for months and years for Last but not least, thanks for creating this amazing library! 😄 |
Given the simple conversion between seconds and minutes/hours/days/weeks I don't think it's worth doing a separate thing for that. If you want to propose some of the desired methods for core, I'd encourage you to go ahead and make a PR or ACP upstream. |
I agree that my proposed changes for Also, I'll create an ACP soon where I'll propose my changes for Also, I just found PR #900 that seems to do what I want regarding the handling of years. I'm looking forward to seeing it in |
@doomlaur AFAIK a key reason that |
Thanks for mentioning that. I was already surprised to be the first person asking for these features - it turns out, I was not. I only now had time to check everything out, like this pull request, as well as this second one, none of which got accepted. Reading these made me notice the following:
Coming from C++ and having used C++20, I think
Some notes:
Note: Note: Some people discussed that a All these changes would make it possible to write applications that, for example, do the following:
Currently, most of this is already possible (thank you very much for that!), however:
What do you think? 😃 |
Sorry, that's way too much elaboration on C++ features particularly in order to support an argument for extending |
I'm sorry, I might have not explained it well - I think supporting Coming back to Not adding convenience functions and/or separate support for leap seconds because of some corner cases would be shame, in my opinion. |
@doomlaur - with regards to durations of longer than a second, we already have precedent for supporting them: If it became clear that there were realistic use cases for similar constructs either of In your example of weeks I think you would be best to use |
Thanks! My bad - although I was aware that I often have use cases like this: take this date and add/subtract a number of years / quarters / months / weeks / days / hours / minutes / seconds / milliseconds / microseconds / nanoseconds, where the number and the time unit can be configured by the user. Which is also the reason why I initially proposed to extend Duration/TimeDelta to use 128-bit integers, as it makes handling microseconds and nanoseconds a lot easier - but switching to Because you mentioned Last but not least, I'm glad that this |
For some use cases I need to work directly with integers instead of chrono::Duration (called chrono::TimeDelta in the not-yet-released chrono 0.5). In such cases I want my code to work reliably with all time units, including microseconds and nanoseconds. Currently, calling
num_microseconds()
ornum_nanoseconds()
returnsOption<i64>
, which requires special handling for these cases, which makes the code more complicated than it has to be.Looking at
std::time::Duration
and attime::Duration
from the time 0.3 crate, they returnu128
andi128
respectively. I know it's possible to use one of these instead, however, chrono::TimeDelta (chrono::Duration in 0.4) is the only duration type which currently works with chrono::DateTime (which is one of the main reasons why I need chrono). I also know that it's possible to convert between std::time::Duration and chrono::Duration, but the conversion function returns aResult
(which makes sense considering it uses unsigned integers), which again makes the code more complicated than it has to be, as special handling is required.Because chrono 0.5 won't depend on time 0.1 at all anymore, I propose the following:
i128
to chrono::TimeDelta in chrono 0.5 ->num_microseconds()
andnum_nanoseconds()
should returni128
. Furthermore,num_milliseconds()
should also returni128
- see my next point.time::Duration
from the time 0.3 crate - that is, having a maximum limit of i64::MAX seconds and 999,999,999 nanoseconds, and a minimum limit of i64::MIN seconds and -999,999,999 nanoseconds.The text was updated successfully, but these errors were encountered: