zeromq / cppzmq

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

Overly Aggressive Deprecation Warnings #586

Open jwmelto opened 1 year ago

jwmelto commented 1 year ago

when most of the legacy API calls were deprecated, "valid" (??) code suddenly raises deprecation warnings.

In particular,

zmq::message_t msg;        // assume some population
sock->send(msg);           // raises deprecation warning

This results from

    ZMQ_CPP11_DEPRECATED("from 4.3.1, use send taking message_t and send_flags")
    bool send(message_t &msg_,
              int flags_ = 0) // default until removed
#ifdef ZMQ_HAS_RVALUE_REFS
    ZMQ_CPP11_DEPRECATED("from 4.3.1, use send taking message_t and send_flags")
    bool send(message_t &&msg_,
              int flags_ = 0) // default until removed

and

    send_result_t send(const_buffer buf, send_flags flags = send_flags::none)
    send_result_t send(message_t &msg, send_flags flags)

(note the inconsistency between buffer and message_t)

But if the new API had been given a reasonable default, there would be no need to touch legacy code:

    ZMQ_CPP11_DEPRECATED("from 4.3.1, use send taking message_t and send_flags")
    bool send(message_t &msg_,
              int flags_); // only when explicitly provided
#ifdef ZMQ_HAS_RVALUE_REFS
    ZMQ_CPP11_DEPRECATED("from 4.3.1, use send taking message_t and send_flags")
    bool send(message_t &&msg_,
              int flags_);
#endif

    send_result_t send(const_buffer buf, send_flags flags = send_flags::none)
    send_result_t send(message_t &msg, send_flags flags = send_flags::none)

Of course, there is an API issue where legacy send could return either a bool or a number of bytes (that's bitten me before), and the new API returns a std::optional<size_t> but for that kind of breaking change, it probably would have been better to not overload the function name.

I realize this change is now quite old (324b11f239511), but maintainers of legacy code are now faced with non-intuitive modifications, when more careful migration planning might have addressed it.

Note that the conversion of std::optional<T> to bool probably gives the desired results (I haven't thought it completely through), so it's probable that the new API is drop-in replacable. The main point should be to minimize breaking changes. Deprecating "normal" code seems to be contrary to that aim.

gummif commented 1 year ago

Yes this is an unfortunate result of the API change. Warnings are better than breaking the code (who knows what the users do with the result of the function).

A possible solution would be to add a macro CPPZMQ_NO_DEPRECATED where we (maybe) break things but the API is cleaner with better defaults. Would that be acceptable in your case? Or just not deprecate that particular function?