Interesting code in http_client.impl.h, pass by value of objects

Nov 27, 2014 at 4:23 PM
Hi,

Here is a code section in this unit, I try to fully understand:
http_client::http_client(uri base_uri)
{
    build_pipeline(std::move(base_uri), http_client_config());
}

http_client::http_client(uri base_uri, http_client_config client_config)
{
    build_pipeline(std::move(base_uri), std::move(client_config));
}

void http_client::build_pipeline(uri base_uri, http_client_config client_config)
{
    if (base_uri.scheme().empty())
    {
        auto uribuilder = uri_builder(base_uri);
        uribuilder.set_scheme(_XPLATSTR("http"));
        base_uri = uribuilder.to_uri();
    }
    details::verify_uri(base_uri);

    m_pipeline = ::web::http::http_pipeline::create_pipeline(std::make_shared<details::http_network_handler>(std::move(base_uri), std::move(client_config)));
// more code 
}
So 'http_client::http_client' takes both params by value. Before C++ 11 this was ok ONLY for basic types (int, double, etc). However, yes there is a reason to do this in C++ 11 WHEN you know that the paramer(s) will be copied inside the function. In that case you use the std::move (does not 'move' anything, just changes to the param to a rvalue) and you get the most efficient code.
Now, inside 'http_client::http_client' the copy is moved again as a param to build_pipeline. This should be ok if the param of this function is a && but it is just another pass by value.
So, I think another copy is made locally in build_pipeline. Inside this function the param is used and then another move is used to pass it to 'create_pipeline'. This time the desired effect is obtained because this function takes the param as a && (rvalue).

Thanks,

G.
Dec 5, 2014 at 3:07 AM
Hi G,

Yes I think a 'mess' of premature optimization and layer complexity has occurred here. What should really be done is just have the constructor take by const reference and avoid all these moves/copies. Then if this was a performance critical path a constructor overload taking by r-value reference could be added.

I have a change prepared that I'm running through review and testing that will make the parameters all be const reference. It will be fixed in the 2.4.0 release. This is the cleanest option and allows for better future improvement.

Thanks for pointing out,
Steve
Dec 5, 2014 at 3:32 AM
Hi Steve,

Yes passing by const& will solve for the moment this issue. The alternative will be to create as you said, move constructors(&&) for all objects involved.

I kind of like this new C++ 11 way of passing parameters by value for objects. However this works ONLY if the object will be copied internally. When the passing parameter is created as a rvalue, this technique saves a copy.

Thanks,

G.
Dec 6, 2014 at 1:26 AM
Hi G,

FYI this is now fixed in the development and will be in release 2.4.0.

Steve