Repository navigation
Add std::chrono::time_point constructor overloads and tests - #50
yalagadapavankumar wants to merge 3 commits into
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
|
Hi teor, i thought using just checklist is more convenient, as it makes easier to what completed. Do you think it's better to include whole PR template? |
This comment was marked as resolved.
This comment was marked as resolved.
This is now fixed, and if people accidentally introduce themselves somewhere else, someone from the community will move their post to the right channel. |
teor2345
left a comment
There was a problem hiding this comment.
Sorry, the instructions weren't clear, we want to see 3-5 different overloaded function names, wrapping all the overloads for each name.
I have updated the PR template and the checklist at the top of this PR with the new wording.
You can choose whatever overloaded functions you like, but I suggest:
| #include <chrono> | ||
|
|
||
| using TestClock = std::chrono::system_clock; | ||
| using TestDuration = std::chrono::milliseconds; |
There was a problem hiding this comment.
This specific type alias makes the code harder to read, because it hides that the type is milliseconds.
Instead, delete this alias, and use the milliseconds type directly.
Please also rename TestTimePoint so it is clear it is in milliseconds.
|
|
||
| /// A wrapper around a C++ time point with millisecond precision. | ||
| #[cfg_attr(not(test), expect(dead_code, reason = "Only used in tests"))] | ||
| pub struct StdTimePoint(*mut c_void, PhantomData<()>); |
There was a problem hiding this comment.
As above, hiding the units makes the type error-prone to use.
| pub struct StdTimePoint(*mut c_void, PhantomData<()>); | |
| pub struct StdMillisecondsTimePoint(*mut c_void, PhantomData<()>); |
| #[cfg_attr(not(test), expect(dead_code, reason = "Only used in tests"))] | ||
| impl StdSecondsTimePoint { | ||
| /// Construct a time point from seconds since the epoch. | ||
| fn from_seconds(seconds: i64) -> Self { |
There was a problem hiding this comment.
The i64 to long long type conversion may fail on some platforms. Instead, use the Rust type that is guaranteed to be a long long on all platforms:
https://doc.rust-lang.org/std/ffi/type.c_longlong.html
Please make this change throughout this file.
Add C++
std::chrono::time_pointconstructors in Rust usingoverload!macro.Limitationsdoc sectionErgonomicsdoc sectionIncompatibilitiesdoc sectionWorkaroundsdoc sectionInexpressible Overloadsdoc section