zeromq / cppzmq

Header-only C++ binding for libzmq
http://www.zeromq.org
MIT License
1.94k stars 757 forks source link

Return type of `sock.get(zmq::sockopt::type)` #522

Open jasujm opened 2 years ago

jasujm commented 2 years ago

The new sock.get(zmq::sockopt::option) API does a good job in retrieving socket options in a typesafe manner, and is definitely an improvement over the old sock.getsockopt() API.

But I think that sock.get(zmq::sockopt::type) in particular works illogically. Currently, this won't compile:

$ cat test.cc 
#include <zmq.hpp>

#include <cassert>

int main()
{
    zmq::context_t ctx;
    zmq::socket_t sock(ctx, zmq::socket_type::push);
    assert(sock.get(zmq::sockopt::type) == zmq::socket_type::push);
    return 0;
}
$ g++ -c test.cc 
In file included from /usr/include/c++/10/cassert:44,
                 from test.cc:3:
test.cc: In function ‘int main()’:
test.cc:9:41: error: no match for ‘operator==’ (operand types are ‘int’ and ‘zmq::socket_type’)
    9 |     assert(sock.get(zmq::sockopt::type) == zmq::socket_type::push);
      |            ~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^~ ~~~~~~~~~~~~~~~~~~~~~~
      |                    |                                         |
      |                    int                                       zmq::socket_type

Would it make more sense that in C++11 and above sock.get(zmq::sockopt::type) would return enumerator from zmq::socket_type instead of an int? int is neither typesafe nor used to represent socket types elsewhere in the library. This change would break backward compatibility slightly, but in a way that I'm sure any user of zmq::sockopt::type would accept.

What do you think?

gummif commented 2 years ago

Yes it looks like we overlook that case. It would be easy to add a new socket_type case to the sockopt enum that would return socket_type on get.

jasujm commented 2 years ago

That sounds good :). I created a PR which would do that: https://github.com/zeromq/cppzmq/pull/523