stream::write - should be implemented in terms of write(char*, int)

[steve-lorimer]

Duplication of code in write:

bool write(const std::vector<char>& buf, ...) and bool write(const std::string& buf, ...) should just defer to bool write(const char* buf, int len, ...)

    bool write(const std::string& buf, std::function<void(error)> callback)
    {
        return write(buf.c_str(), buf.length(), callback);
    }

    bool write(const std::vector<char>& buf, std::function<void(error)> callback)
    {
        return write(&buf[0], buf.size(), callback);
    }

[jalcine]

Question: What's the point of having a std::vector if we can just use a std::basic_string?

[larroy]

Makes no difference, we could have both methods, or just one with a generic pointer and size, but it's less secure

[aggsol]

Actually having a vector of widechars can be beneficial for using UTF-8/16 strings for CJK languages.

[jalcine]

Right but wouldn't you just use a std::wstring in that case?

[larroy]

I think that's problematic, the best way in my opinion is to always have utf-8 encoded strings, so either plain std::string or vector wchar_t and thus wstring shouldn't be used in public interfaces, the size of wchar_t is implementation defined.

http://en.wikipedia.org/wiki/Wide_character

[steve-lorimer]

When I submitted this issue I wasn't talking about changing the signatures of the various overloads or whether to add or remove any, I was alluding to code duplication.

Currently this is the code:

    bool write(const char* buf, int len, std::function<void(error)> callback)
    {
        uv_buf_t bufs[] = { uv_buf_t { const_cast<char*>(buf), static_cast<size_t>(len) } };
        callbacks::store(handle<HANDLE_T>::get()->data, uvpp::internal::uv_cid_write, callback);
        return uv_write(new uv_write_t, handle<HANDLE_T>::template get<uv_stream_t>(), bufs, 1, [](uv_write_t* req, int status) {
            callbacks::invoke<decltype(callback)>(req->handle->data, uvpp::internal::uv_cid_write, error(status));
            delete req;
        }) == 0;
    }

    bool write(const std::string& buf, std::function<void(error)> callback)
    {
        uv_buf_t bufs[] = { uv_buf_t { const_cast<char*>(buf.c_str()), buf.length()} };
        callbacks::store(handle<HANDLE_T>::get()->data, uvpp::internal::uv_cid_write, callback);
        return uv_write(new uv_write_t, handle<HANDLE_T>::template get<uv_stream_t>(), bufs, 1, [](uv_write_t* req, int status) {
            callbacks::invoke<decltype(callback)>(req->handle->data, uvpp::internal::uv_cid_write, error(status));
            delete req;
        }) == 0;
    }

    bool write(const std::vector<char>& buf, std::function<void(error)> callback)
    {
        uv_buf_t bufs[] = { uv_buf_t { const_cast<char*>(&buf[0]), buf.size() } };
        callbacks::store(handle<HANDLE_T>::get()->data, uvpp::internal::uv_cid_write, callback);
        return uv_write(new uv_write_t, handle<HANDLE_T>::template get<uv_stream_t>(), bufs, 1, [](uv_write_t* req, int status) {
            callbacks::invoke<decltype(callback)>(req->handle->data, uvpp::internal::uv_cid_write, error(status));
            delete req;
        }) == 0;
    }

and I think it would be better written like this:

    bool write(const char* buf, int len, std::function<void(error)> callback)
    {
        uv_buf_t bufs[] = { uv_buf_t { const_cast<char*>(buf), static_cast<size_t>(len) } };
        callbacks::store(handle<HANDLE_T>::get()->data, uvpp::internal::uv_cid_write, callback);
        return uv_write(new uv_write_t, handle<HANDLE_T>::template get<uv_stream_t>(), bufs, 1, [](uv_write_t* req, int status) {
            callbacks::invoke<decltype(callback)>(req->handle->data, uvpp::internal::uv_cid_write, error(status));
            delete req;
        }) == 0;
    }

    bool write(const std::string& buf, std::function<void(error)> callback)
    {
        return write(buf.data(), buf.size(), callback);
    }

    bool write(const std::vector<char>& buf, std::function<void(error)> callback)
    {
        return write(&buf[0], buf.size(), callback);
    }        

Note also that you're passing the callback by value, so it would probably be better to use perfect forwarding too:

    bool write(const char* buf, int len, std::function<void(error)>&& callback)
    {
        uv_buf_t bufs[] = { uv_buf_t { const_cast<char*>(buf), static_cast<size_t>(len) } };
        callbacks::store(handle<HANDLE_T>::get()->data, uvpp::internal::uv_cid_write, callback);
        return uv_write(new uv_write_t, handle<HANDLE_T>::template get<uv_stream_t>(), bufs, 1, [](uv_write_t* req, int status) {
            callbacks::invoke<decltype(callback)>(req->handle->data, uvpp::internal::uv_cid_write, error(status));
            delete req;
        }) == 0;
    }

    bool write(const std::string& buf, std::function<void(error)>&& callback)
    {
        return write(buf.data(), buf.size(), std::forward<std::function<void(error)>>(callback));
    }

    bool write(const std::vector<char>& buf, std::function<void(error)>&& callback)
    {
        return write(&buf[0], buf.size(), std::forward<std::function<void(error)>>(callback));
    }        

[jalcine]

@stevelorimer heh, should have caught that smell. That'd make a useful PR.

[larroy]

👍