Skip to content
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

Remove redundant static_cast<void*> in examples. #859

Merged

Conversation

TobiSchluter
Copy link

@TobiSchluter TobiSchluter commented Jul 27, 2021

Not only is it unnecessary because zmq::socket_t automatically converts
to void* via operator void*, it is also misleading as the operator is
called in spite of the appearance that it's "just" a cast.

Now, other comments in zmq.hpp indicate that this is not really the
intent (note the comments near the bool conversion), and I believe the
cleanest fix would be to add a member to zmq::socket_t that returns the
void* needed for polling (.pollable()? I really can't find a good name),
or for zmq::pollitem_t to handle the conversion instead of having this
magical operator. But since this attempts to document what's there, I
think we should document what is there now while avoiding confusing
bits. As it is there is a C++ type zmq::pollitem_t which can be filled
from zmq::socket_t without jumping through hoops, so let's document
that even though one could infer from the man page for zmq_poll that
a static_cast<void*> is needed.

This PR is related to #858

Not only is it unnecessary because zmq::socket_t automatically converts
to void* via operator void*, it is also misleading as the operator is
called in spite of the appearance that it's just a cast.

Now, other comments in zmq.hpp indicate that this is not really the
intent (note the comments near the bool conversion), and I believe the
cleanest fix would be to add a member to zmq::socket_t that returns the
void* needed for polling (.pollable()? I really can't find a good name),
or for zmq::pollitem_t to handle the conversion transparently.
@TobiSchluter
Copy link
Author

Well all these failing checks seem to complain about C# in a filename in parts that I didn't touch but maybe I misunderstand?

@sappo sappo merged commit 55d417d into booksbyus:master Jul 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants