8

My understanding is that in C++17, the following snippet is intended to Do The Right Thing:

struct Instrument;  // instrumented (non-trivial) move and copy operations

struct Base {
    Instrument i;
};

struct Derived : public Base {};

struct Unrelated {
    Instrument i;
    Unrelated(const Derived& d): i(d.i) {}
    Unrelated(Derived&& d): i(std::move(d.i)) {}
};

Unrelated test1() {
    Derived d1;
    return d1;
}

Base test2() {
    Derived d2;
    return d2;  // yes, this is slicing!
}

That is, in C++17, the compiler is supposed to treat both d1 and d2 as rvalues for the purposes of overload resolution in those two return statements. However, in C++14 and earlier, this was not the case; the lvalue-to-rvalue transformation in return operands was supposed to apply only when the operand was exactly the correct return type.

Furthermore, GCC and Clang both appear to have confusing and possibly buggy behavior in this area. Trying the above code on Wandbox, I see these outputs:

GCC 4.9.3 and earlier: copy/copy (regardless of -std=)
Clang 3.8.1 and earlier: copy/copy (regardless of -std=)
Clang 3.9.1 and later: move/copy (regardless of -std=)
GCC 5.1.0 through 7.1.0: move/copy (regardless of -std=)
GCC 8.0.1 (HEAD): move/move (regardless of -std=)

So this started out as a tooling question and ended up with a side order of "what the heck is the correct behavior for a C++ compiler?"

My tooling question is: In our codebase, we have several places that say return x; but that accidentally produces a copy instead of a move because our toolchain is GCC 4.9.x and/or Clang. We'd like to detect this situation automatically and insert std::move() as needed. Is there any easy way to detect this issue? Maybe a clang-tidy check or a -Wfoo flag we could enable?

But of course now I'd also like to know what is the correct behavior of a C++ compiler on this code. Are these outputs indicative of GCC/Clang bugs? Are they being worked on? And is the language version (-std=) supposed to matter? (I'd think that it is supposed to matter, unless the correct behavior has been updated via Defect Reports going all the way back to C++11.)

Here is a more complete test inspired by Barry's answer. We test six different cases where lvalue-to-rvalue conversion would be desirable.

GCC 4.9.3 and earlier:   elided/copy/copy/copy/copy/copy
Clang 3.8.1 and earlier: elided/copy/copy/copy/copy/copy
Clang 3.9.1 and later:   elided/copy/move/copy/copy/copy
GCC 5.1.0 through 7.1.0: elided/copy/move/move/move/move
GCC 8.0.1 (HEAD):        elided/move/move/move/move/move

ICC 17:                  elided/copy/copy/copy/copy/copy  
ICC 18:                  elided/move/move/move/copy/copy
MSVC 2017 (wow):         elided/copy/move/copy/copymove/copymove

After Barry's answer, it seems to me that Clang 3.9+ does the technically correct thing in all cases; GCC 8+ does the desirable thing in all cases; and in general I ought to stop teaching that people "just return x and let the compiler DTRT" (or at least teach it with a huge flashing caveat) because in practice the compiler will not DTRT unless you are using a bleeding-edge (and technically non-conforming) GCC.

Quuxplusone
  • 23,928
  • 8
  • 94
  • 159
  • 3
    Please look at https://stackoverflow.com/questions/20674231/gcc-nrvo-rvo-warning Maybe it would help. – 273K Feb 11 '18 at 02:56
  • In my code base, I've been scrubbing the code that is doing exactly the kind of slicing that test2 is doing. But for my code base, it was unintentional (and, basically, wrong) in every instance. – Eljay Feb 11 '18 at 03:01

1 Answers1

11

The correct behavior is move/copy. You probably want to just write a clang-tidy check.


The wording in C++17 is [class.copy.elision]/3 and in C++14 is [class.copy]/32. The specific words and formatting is different but the rule is the same.

In C++11, the rule wording was [class.copy]/32 and was tied to the copy elision rules, the exception for automatic storage local variables was added in CWG 1579 as a defect report. Compilers predating this defect report would behave as copy/copy. But as the defect report is against C++11, compilers implementing the wording change would implement it across all standard versions.

Using the C++17 wording:

In the following copy-initialization contexts, a move operation might be used instead of a copy operation:

  • If the expression in a return statement is a (possibly parenthesized) id-expression that names an object with automatic storage duration declared in the body or parameter-declaration-clause of the innermost enclosing function or lambda-expression, or
  • [ ... ]

overload resolution to select the constructor for the copy is first performed as if the object were designated by an rvalue. If the first overload resolution fails or was not performed, or if the type of the first parameter of the selected constructor is not an rvalue reference to the object's type (possibly cv-qualified), overload resolution is performed again, considering the object as an lvalue.

In:

Unrelated test1() {
    Derived d1;
    return d1;
}

We meet the first bullet, so we try to copy-initialize an Unrelated with an rvalue of type Derived, which gives us Unrelated(Derived&& ). That meets the highlighted criteria so we use it, and the result is a move.

In:

Base test2() {
    Derived d2;
    return d2;  // yes, this is slicing!
}

We again meet the first bullet, but overload resolution will find Base(Base&& ). The first parameter of the selected constructor is not an rvalue reference to Derived (possibly cv-qualified), so we perform overload resolution again - and end up copying.

Barry
  • 286,269
  • 29
  • 621
  • 977
  • Hmm, so `d2` won't DTRT even in C++17, eh? And if I had an `Unrelated2` whose only relevant constructors were `Unrelated2(const Base&)` and `Unrelated2(Base&&)`, the compiler would pick the `Unrelated2(const Base&)` overload instead of DTRT? (I believe your response does answer these questions unambiguously in the affirmative; I'm just acting incredulous for effect. :)) – Quuxplusone Feb 11 '18 at 04:11
  • And orthogonally: The rule has never changed? Clang pre-3.9 was buggy, and GCC pre-5.1 was buggy, and GCC trunk is buggy as well? (Although I like GCC trunk's behavior, and hope the standard will catch up to it.) – Quuxplusone Feb 11 '18 at 04:12
  • This was a DR against C++11. – T.C. Feb 11 '18 at 09:52
  • @T.C. Do you know which one? I tried searching but couldn't find anything – Barry Feb 11 '18 at 14:38
  • @T.C. How... did I not find that. ::confused:: Thanks! – Barry Feb 11 '18 at 23:25
  • @Barry: Wait, after reading [this](https://stackoverflow.com/a/21241437/1424877) I think this answer is incomplete. In C++11, class.copy/32 said "When the criteria for elision of a copy operation are met..." and the criteria for copy elision required ["the same cv-unqualified type as the function return type"](https://timsong-cpp.github.io/cppwp/n3337/class.copy#31.1). Whereas in C++14 the same-type requirement was removed, maybe via [CWG 1579](http://www.open-std.org/jtc1/sc22/wg21/docs/cwg_defects.html#1579), I can't 100% tell. Does CWG 1579 change C++11, or does it apply only to C++14? – Quuxplusone Feb 12 '18 at 17:56
  • And, orthogonally: All of this class.copy wording is talking about *constructors*, right? None of it would apply to finding an overloaded conversion operator such as `Derived::operator Unrelated3() &&`, is that right? (I'm gonna edit some of these into my question for completeness.) – Quuxplusone Feb 12 '18 at 17:58
  • 1
    @Quuxplusone The defect report is against C++11. Of course, compilers that predate CWG 1579 would not implement it. And yes, it's about constructors. – Barry Feb 12 '18 at 18:21