-
Notifications
You must be signed in to change notification settings - Fork 790
Add duration template for all function using std::chrono::milliseconds #666
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
base: master
Are you sure you want to change the base?
Conversation
Pull Request Test Coverage Report for Build 18631924080Details
💛 - Coveralls |
d2be5ed to
79b3bd2
Compare
|
Not sure how to unit test this properly.. Is there a "check if compiles" kind of unit tests for catch2? |
|
|
||
| #ifdef ZMQ_CPP11 | ||
| template<typename Duration = std::chrono::milliseconds> | ||
| bool check_event(Duration timeout = std::chrono::milliseconds{0}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
check_event() is ambiguous
| { | ||
| return detail::poll(items, nitems, static_cast<long>(timeout.count())); | ||
| auto timeout_ms = std::chrono::duration_cast<std::chrono::milliseconds>(timeout); | ||
| return detail::poll(items, nitems, static_cast<long>(timeout_ms.count())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One concern is that for poll and poller, -1 millisecond is a special value that may need special handling. E.g. does -1s or -1ns do the right thing?
If the value of timeout is -1, zmq_poll() shall block indefinitely until a requested event has occurred on at least one zmq_pollitem_t.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Negative duration work:
#include <print>
#include <optional>
#include <chrono>
int main()
{
std::chrono::milliseconds ms{-1};
std::println("{}", ms);
std::println("{}", ms.count());
std::chrono::nanoseconds ns{-1};
std::println("{}", ns);
std::println("{}", ns.count());
return 0;
}Output:
$ g++ -std=c++23 temp.cpp && ./a.out
-1ms
-1
-1ns
-1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With c++17:
int main()
{
std::chrono::milliseconds ms{-1};
// std::cout << ms << '\n';
std::cout << ms.count() << '\n';
std::chrono::nanoseconds ns{-1};
// std::cout << ns << '\n';
std::cout << ns.count() << '\n';
return 0;
}Output:
$ g++ -std=c++17 temp.cpp && ./a.out
-1
-1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but you cast the input Duration to milliseconds then pass that to libzmq.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, casting from -1ns to ms give 0ms, I'll change that
|
To check if something compiles or not you could use C++20/23 and concept or requires expressions. |
…ce the template would be a breaking change in C++17 and bellow
Makes the bindings slightly easier to use when dealing with durations. With the bindings handling the cast instead of the user