CppCoreGuidelines icon indicating copy to clipboard operation
CppCoreGuidelines copied to clipboard

F.21: Wording might invite large copies when replacing "out" parameters by returned tuples

Open nh2 opened this issue 3 years ago • 3 comments

F.21: To return multiple “out” values, prefer returning a struct or tuple currently suggests:

// GOOD: self-documenting
tuple<int, string> f(const string& input)
{
    // ...
    return make_tuple(status, something());
}

I find the wording and the example a bit contrived, because: The type of int status and the construction of the string something() are chosen here such that performance is good (the int is cheap and the string can be constructed directly in the tuple), but when one follows the general pattern of return make_tuple(a, b); large copies can occur that would not occur with "out" parameters.

Concretely:

tuple<LargeObject, LargeObject> f(const string& input)
{
    LargeObject large1;
    LargeObject large2;
    // ...
    return make_tuple(large1, large2);
}

Following the guideline this way would be a pessimisation of over just using "out" parameters, because the cost of copying the values into the tuple is high.

I wonder if/how F.21's wording could be changed so that does not invite this mistake.

nh2 avatar Jul 24 '22 12:07 nh2

I guess the best course of action depends on the concrete case. Two possiblilities that come to mind are

  • Use std::move, if the ype(s) are cheap to move

  • Construct the tuple at the beginning

    tuple<LargeObject, LargeObject> f(const string& input) { std::tuple<LargeObject,LargeObject> ret_val; // ... return ret_val; }

As a sidenote: Outside of generic code, I wouldn't use a tuple, but a struct with named members.

MikeGitb avatar Jul 24 '22 13:07 MikeGitb

Yes, I agree that those are two sane choices. I wonder whether they should be suggested in F.21, or whether it would extend it too much.

A separate annoyance about "Construct the tuple at the beginning" is that it while it eliminates the large copies (good), it turns the behaviour from RVO (guaranteed copy elision of the return in C++17) into NVRO (commonly implemented but not guaranteed), thus making it potentially slightly less efficient than "out" parameters, and in some cases makes it having different behaviour than "out" parameters (for example if you return a tuple of (someVector, pointersIntoThatVectorsMemory)) -- which makes the transformation from "out" parameters into "construct tuple at beginning and return it" less generally applicable.

My sidenote: I find it a bit frustrating that in current C++ we have to choose between (A) nicest syntax / ease of construction / maximum obviousness (return make_tuple(...)), (B) the requirement for move constructors and the move being cheap (std::move), and (C) the non-guaranteedness of NVRO.

nh2 avatar Jul 24 '22 14:07 nh2

IMO nicest syntax is not even return make_tuple(...);, but simply return {a, b}; Which, in this case, indeed has to become return {move(large1), move(large2)}; to avoid copy ctors.

cubbimew avatar Aug 08 '22 19:08 cubbimew

Editors call: We should

  • change return make_tuple(a,b); to return {a,b};
  • mention that returning large objects may move (e.g., return {std::move(a), std::move(b)};).
  • in the get_string example, where we already have return {is,s}; change this to return {is, std::move(s)}; and this is the last use so the move is safe
  • see also the rule not to do a direct return std::move(...) (ES.56)

hsutter avatar Sep 22 '22 21:09 hsutter