zeromq / cppzmq

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

Rebuild with string_view argument, like the constructor #584

Open Crayon2000 opened 1 year ago

Crayon2000 commented 1 year ago

More convenient, like the constructor message_t(std::string_view str)

This is similar to PR #579 but with std::string_view instead of std::string&.

gummif commented 1 year ago

Could you add a test case with string literal like m.rebuild("hi")?

Crayon2000 commented 1 year ago

Sorry for all the back and forth. I thought the CI would run automatically when the PR was submitted. I didn't expect you to be my compiler.

gummif commented 1 year ago

Thats ok. For some reason I have to approve after each commit.

Crayon2000 commented 1 year ago

I'm a first-time contributor, so you need to approve the workflow.

Crayon2000 commented 1 year ago

@gummif, the test you asked to add with hi_msg.rebuild("Hi") is failing:

/home/runner/work/cppzmq/cppzmq/tests/message.cpp:193:24: error: call of overloaded ‘rebuild(const char [3])’ is ambiguous
  193 |     hi_msg.rebuild("Hi");
      |                        ^
In file included from /home/runner/work/cppzmq/cppzmq/tests/message.cpp:3:
/home/runner/work/cppzmq/cppzmq/zmq.hpp:542:10: note: candidate: ‘void zmq::message_t::rebuild(const string&)’
  542 |     void rebuild(const std::string &str)
      |          ^~~~~~~
/home/runner/work/cppzmq/cppzmq/zmq.hpp:548:10: note: candidate: ‘void zmq::message_t::rebuild(std::string_view)’
  548 |     void rebuild(std::string_view str)
      |          ^~~~~~~

The test works with the constructor because of this implementation:

    // NOTE this constructor will include the null terminator
    // when called with a string literal.
    // An overload taking const char* can not be added because
    // it would be preferred over this function and break compatiblity.
    template<
      class Char,
      size_t N,
      typename = typename std::enable_if<detail::is_char_type<Char>::value>::type>
    ZMQ_DEPRECATED("from 4.7.0, use constructors taking iterators, (pointer, size) "
                   "or strings instead")
    explicit message_t(const Char (&data)[N]) :
        message_t(detail::ranges::begin(data), detail::ranges::end(data))
    {
    }

I don't think this a good idea to add this for rebuild.

So, should I remove the test for string literal? Or you want me to test my new implementation for string_view with hi_msg.rebuild("Hi"sv) (https://en.cppreference.com/w/cpp/string/basic_string_view/operator%22%22sv)? Or anything else?

gummif commented 1 year ago

So what I was thinking is that currently there is only an overload for std::string so rebuild("foo") should work today. Then you added the overload for string_view so no there is an ambiguity and could break existing code. So I think the solution is to add another overload

void rebuild(const char* p)
{
  rebuild(p, std::strlen(p));
}

How does that sound?