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

Rebuild with string_view argument, like the constructor #584

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Crayon2000
Copy link

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
Copy link
Member

gummif commented Nov 2, 2022

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

tests/message.cpp Outdated Show resolved Hide resolved
tests/message.cpp Outdated Show resolved Hide resolved
@Crayon2000
Copy link
Author

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
Copy link
Member

gummif commented Nov 4, 2022

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

@Crayon2000
Copy link
Author

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

@Crayon2000
Copy link
Author

@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
Copy link
Member

gummif commented Nov 7, 2022

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?

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