Automatically detect C++14 "return should use std::move" situation
Asked Answered
W

1

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.

2023 update

Since I submitted this question in 2018, C++ has evolved (in this area mainly due to two papers by me and one by David Stone), such that the paper and practical behaviors are much more in keeping with what I originally expected. The more complete test above now produces moves on the most recent GCCs and Clangs, in all language modes:

GCC 8, all stds:         elided/move/move/move/move/move
GCC 9-10, all stds:      elided/copy/move/move/move/move
GCC 12, -std < 20:       elided/copy/move/copy/move/move
GCC 12, -std >= 20:      elided/move/move/move/move/move
GCC 13+, all stds:       elided/move/move/move/move/move

Clang 12, all stds:      elided/copy/move/copy/copy/copy
Clang 13, all stds:      elided/move/move/move/move/move
Wholesale answered 11/2, 2018 at 2:47 Comment(2)
Please look at https://mcmap.net/q/553191/-gcc-nrvo-rvo-warning Maybe it would help.Drove
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.Photoengrave
P
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.

Parkin answered 11/2, 2018 at 3:7 Comment(8)
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. :))Wholesale
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.)Wholesale
This was a DR against C++11.Ellaelladine
@Ellaelladine Do you know which one? I tried searching but couldn't find anythingParkin
@Ellaelladine How... did I not find that. ::confused:: Thanks!Parkin
@Barry: Wait, after reading this 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". Whereas in C++14 the same-type requirement was removed, maybe via CWG 1579, I can't 100% tell. Does CWG 1579 change C++11, or does it apply only to C++14?Wholesale
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.)Wholesale
@Wholesale The defect report is against C++11. Of course, compilers that predate CWG 1579 would not implement it. And yes, it's about constructors.Parkin

© 2022 - 2024 — McMap. All rights reserved.