New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

inplace_function implicit conversion chooses copy over move #125

Open
Quuxplusone opened this Issue Feb 14, 2018 · 1 comment

Comments

Projects
None yet
2 participants
@Quuxplusone
Copy link
Contributor

Quuxplusone commented Feb 14, 2018

This is a really unfortunate side-effect of a core language issue, IMNSHO; this week I have been actively working on trying to understand and diagnose the core issue. CWG 1579 is related but its resolution left much to be desired.

Consider this program:

using IPF20 = stdext::inplace_function<void(), 20>;
using IPF40 = stdext::inplace_function<void(), 40>;
static_assert(std::is_convertible<IPF20, IPF40>::value, "");

IPF40 foo() {
    IPF20 f;
    return f;  // HERE
}

What happens on the line marked "HERE"? Naively, we would expect NRVO (copy elision). But that can't happen because IPF20 and IPF40 are not the same type. So, post-CWG1579, we would expect lvalue-to-rvalue conversion — we'd expect that the expression f is treated as an rvalue, and the wrapped functor is moved from f into the return slot. However, CWG1579 kicks in only for constructors; it does not kick in for inplace_function's clever use of a templated conversion operator to implement implicit conversions. So CWG1579 does not kick in, and what we actually get in this case is a copy operation.

SG14 could fix this by changing the implementation of implicit conversions for inplace_function to use converting constructors instead of conversion operators, but this strikes me as a library fix for a language bug. I am pessimistic but not fatalistic that we might actually be able to get a core language fix here. I'm opening this GitHub issue just to document the issue for posterity.

(I remember remarking on @Voultapher's unusual and clever use of conversion operators at the time. He convinced me that it was the Right Way to do it, and I still think it is, really. But I'm sad to see that it actually does have minor negative consequences due to this core language bug.)

@mattreecebentley

This comment has been minimized.

Copy link
Contributor

mattreecebentley commented Feb 14, 2018

Thanks for making me look up IMNSHO :)
Assumption of copy elision never works out well, in my experience. While it'd be nice to be able to rely on compilers and the language always doing the right thing, it's better to brute-force it if you always want the right outcome.

jyknight pushed a commit to jyknight/llvm-monorepo that referenced this issue Apr 12, 2018

Diagnose cases of "return x" that should be "return std::move(x)" for…
… efficiency

Summary:
This patch adds two new diagnostics, which are off by default:

**-Wreturn-std-move**

This diagnostic is enabled by `-Wreturn-std-move`, `-Wmove`, or `-Wall`.
Diagnose cases of `return x` or `throw x`, where `x` is the name of a local variable or parameter, in which a copy operation is performed when a move operation would have been available. The user probably expected a move, but they're not getting a move, perhaps because the type of "x" is different from the return type of the function.
A place where this comes up in the wild is `stdext::inplace_function<Sig, N>` which implements conversion via a conversion operator rather than a converting constructor; see WG21-SG14/SG14#125 (comment)
Another place where this has come up in the wild, but where the fix ended up being different, was

    try { ... } catch (ExceptionType ex) {
        throw ex;
    }

where the appropriate fix in that case was to replace `throw ex;` with `throw;`, and incidentally to catch by reference instead of by value. (But one could contrive a scenario where the slicing was intentional, in which case throw-by-move would have been the appropriate fix after all.)
Another example (intentional slicing to a base class) is dissected in https://github.com/accuBayArea/Slides/blob/master/slides/2018-03-07.pdf

**-Wreturn-std-move-in-c++11**

This diagnostic is enabled only by the exact spelling `-Wreturn-std-move-in-c++11`.
Diagnose cases of "return x;" or "throw x;" which in this version of Clang *do* produce moves, but which prior to Clang 3.9 / GCC 5.1 produced copies instead. This is useful in codebases which care about portability to those older compilers.
The name "-in-c++11" is not technically correct; what caused the version-to-version change in behavior here was actually CWG 1579, not C++14. I think it's likely that codebases that need portability to GCC 4.9-and-earlier may understand "C++11" as a colloquialism for "older compilers." The wording of this diagnostic is based on feedback from @rsmith.

**Discussion**

Notice that this patch is kind of a negative-space version of Richard Trieu's `-Wpessimizing-move`. That diagnostic warns about cases of `return std::move(x)` that should be `return x` for speed. These diagnostics warn about cases of `return x` that should be `return std::move(x)` for speed. (The two diagnostics' bailiwicks do not overlap: we don't have to worry about a `return` statement flipping between the two states indefinitely.)

I propose to write a paper for San Diego that would relax the implicit-move rules so that in C++2a the user //would// see the moves they expect, and the diagnostic could be re-worded in a later version of Clang to suggest explicit `std::move` only "in C++17 and earlier." But in the meantime (and/or forever if that proposal is not well received), this diagnostic will be useful to detect accidental copy operations.

Reviewers: rtrieu, rsmith

Reviewed By: rsmith

Subscribers: lebedev.ri, Rakete1111, rsmith, cfe-commits

Tags: #clang

Differential Revision: https://reviews.llvm.org/D43322

Patch by Arthur O'Dwyer.

llvm-svn=329914

chapuni pushed a commit to llvm-project/llvm-project-20170507 that referenced this issue Apr 12, 2018

Diagnose cases of "return x" that should be "return std::move(x)" for…
… efficiency

Summary:
This patch adds two new diagnostics, which are off by default:

**-Wreturn-std-move**

This diagnostic is enabled by `-Wreturn-std-move`, `-Wmove`, or `-Wall`.
Diagnose cases of `return x` or `throw x`, where `x` is the name of a local variable or parameter, in which a copy operation is performed when a move operation would have been available. The user probably expected a move, but they're not getting a move, perhaps because the type of "x" is different from the return type of the function.
A place where this comes up in the wild is `stdext::inplace_function<Sig, N>` which implements conversion via a conversion operator rather than a converting constructor; see WG21-SG14/SG14#125 (comment)
Another place where this has come up in the wild, but where the fix ended up being different, was

    try { ... } catch (ExceptionType ex) {
        throw ex;
    }

where the appropriate fix in that case was to replace `throw ex;` with `throw;`, and incidentally to catch by reference instead of by value. (But one could contrive a scenario where the slicing was intentional, in which case throw-by-move would have been the appropriate fix after all.)
Another example (intentional slicing to a base class) is dissected in https://github.com/accuBayArea/Slides/blob/master/slides/2018-03-07.pdf

**-Wreturn-std-move-in-c++11**

This diagnostic is enabled only by the exact spelling `-Wreturn-std-move-in-c++11`.
Diagnose cases of "return x;" or "throw x;" which in this version of Clang *do* produce moves, but which prior to Clang 3.9 / GCC 5.1 produced copies instead. This is useful in codebases which care about portability to those older compilers.
The name "-in-c++11" is not technically correct; what caused the version-to-version change in behavior here was actually CWG 1579, not C++14. I think it's likely that codebases that need portability to GCC 4.9-and-earlier may understand "C++11" as a colloquialism for "older compilers." The wording of this diagnostic is based on feedback from @rsmith.

**Discussion**

Notice that this patch is kind of a negative-space version of Richard Trieu's `-Wpessimizing-move`. That diagnostic warns about cases of `return std::move(x)` that should be `return x` for speed. These diagnostics warn about cases of `return x` that should be `return std::move(x)` for speed. (The two diagnostics' bailiwicks do not overlap: we don't have to worry about a `return` statement flipping between the two states indefinitely.)

I propose to write a paper for San Diego that would relax the implicit-move rules so that in C++2a the user //would// see the moves they expect, and the diagnostic could be re-worded in a later version of Clang to suggest explicit `std::move` only "in C++17 and earlier." But in the meantime (and/or forever if that proposal is not well received), this diagnostic will be useful to detect accidental copy operations.

Reviewers: rtrieu, rsmith

Reviewed By: rsmith

Subscribers: lebedev.ri, Rakete1111, rsmith, cfe-commits

Tags: #clang

Differential Revision: https://reviews.llvm.org/D43322

Patch by Arthur O'Dwyer.

chapuni pushed a commit to llvm-project/clang that referenced this issue Apr 12, 2018

Diagnose cases of "return x" that should be "return std::move(x)" for…
… efficiency

Summary:
This patch adds two new diagnostics, which are off by default:

**-Wreturn-std-move**

This diagnostic is enabled by `-Wreturn-std-move`, `-Wmove`, or `-Wall`.
Diagnose cases of `return x` or `throw x`, where `x` is the name of a local variable or parameter, in which a copy operation is performed when a move operation would have been available. The user probably expected a move, but they're not getting a move, perhaps because the type of "x" is different from the return type of the function.
A place where this comes up in the wild is `stdext::inplace_function<Sig, N>` which implements conversion via a conversion operator rather than a converting constructor; see WG21-SG14/SG14#125 (comment)
Another place where this has come up in the wild, but where the fix ended up being different, was

    try { ... } catch (ExceptionType ex) {
        throw ex;
    }

where the appropriate fix in that case was to replace `throw ex;` with `throw;`, and incidentally to catch by reference instead of by value. (But one could contrive a scenario where the slicing was intentional, in which case throw-by-move would have been the appropriate fix after all.)
Another example (intentional slicing to a base class) is dissected in https://github.com/accuBayArea/Slides/blob/master/slides/2018-03-07.pdf

**-Wreturn-std-move-in-c++11**

This diagnostic is enabled only by the exact spelling `-Wreturn-std-move-in-c++11`.
Diagnose cases of "return x;" or "throw x;" which in this version of Clang *do* produce moves, but which prior to Clang 3.9 / GCC 5.1 produced copies instead. This is useful in codebases which care about portability to those older compilers.
The name "-in-c++11" is not technically correct; what caused the version-to-version change in behavior here was actually CWG 1579, not C++14. I think it's likely that codebases that need portability to GCC 4.9-and-earlier may understand "C++11" as a colloquialism for "older compilers." The wording of this diagnostic is based on feedback from @rsmith.

**Discussion**

Notice that this patch is kind of a negative-space version of Richard Trieu's `-Wpessimizing-move`. That diagnostic warns about cases of `return std::move(x)` that should be `return x` for speed. These diagnostics warn about cases of `return x` that should be `return std::move(x)` for speed. (The two diagnostics' bailiwicks do not overlap: we don't have to worry about a `return` statement flipping between the two states indefinitely.)

I propose to write a paper for San Diego that would relax the implicit-move rules so that in C++2a the user //would// see the moves they expect, and the diagnostic could be re-worded in a later version of Clang to suggest explicit `std::move` only "in C++17 and earlier." But in the meantime (and/or forever if that proposal is not well received), this diagnostic will be useful to detect accidental copy operations.

Reviewers: rtrieu, rsmith

Reviewed By: rsmith

Subscribers: lebedev.ri, Rakete1111, rsmith, cfe-commits

Tags: #clang

Differential Revision: https://reviews.llvm.org/D43322

Patch by Arthur O'Dwyer.

git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@329914 91177308-0d34-0410-b5e6-96231b3b80d8

chapuni pushed a commit to llvm-project/llvm-project-submodule that referenced this issue Apr 12, 2018

Diagnose cases of "return x" that should be "return std::move(x)" for…
… efficiency

Summary:
This patch adds two new diagnostics, which are off by default:

**-Wreturn-std-move**

This diagnostic is enabled by `-Wreturn-std-move`, `-Wmove`, or `-Wall`.
Diagnose cases of `return x` or `throw x`, where `x` is the name of a local variable or parameter, in which a copy operation is performed when a move operation would have been available. The user probably expected a move, but they're not getting a move, perhaps because the type of "x" is different from the return type of the function.
A place where this comes up in the wild is `stdext::inplace_function<Sig, N>` which implements conversion via a conversion operator rather than a converting constructor; see WG21-SG14/SG14#125 (comment)
Another place where this has come up in the wild, but where the fix ended up being different, was

    try { ... } catch (ExceptionType ex) {
        throw ex;
    }

where the appropriate fix in that case was to replace `throw ex;` with `throw;`, and incidentally to catch by reference instead of by value. (But one could contrive a scenario where the slicing was intentional, in which case throw-by-move would have been the appropriate fix after all.)
Another example (intentional slicing to a base class) is dissected in https://github.com/accuBayArea/Slides/blob/master/slides/2018-03-07.pdf

**-Wreturn-std-move-in-c++11**

This diagnostic is enabled only by the exact spelling `-Wreturn-std-move-in-c++11`.
Diagnose cases of "return x;" or "throw x;" which in this version of Clang *do* produce moves, but which prior to Clang 3.9 / GCC 5.1 produced copies instead. This is useful in codebases which care about portability to those older compilers.
The name "-in-c++11" is not technically correct; what caused the version-to-version change in behavior here was actually CWG 1579, not C++14. I think it's likely that codebases that need portability to GCC 4.9-and-earlier may understand "C++11" as a colloquialism for "older compilers." The wording of this diagnostic is based on feedback from @rsmith.

**Discussion**

Notice that this patch is kind of a negative-space version of Richard Trieu's `-Wpessimizing-move`. That diagnostic warns about cases of `return std::move(x)` that should be `return x` for speed. These diagnostics warn about cases of `return x` that should be `return std::move(x)` for speed. (The two diagnostics' bailiwicks do not overlap: we don't have to worry about a `return` statement flipping between the two states indefinitely.)

I propose to write a paper for San Diego that would relax the implicit-move rules so that in C++2a the user //would// see the moves they expect, and the diagnostic could be re-worded in a later version of Clang to suggest explicit `std::move` only "in C++17 and earlier." But in the meantime (and/or forever if that proposal is not well received), this diagnostic will be useful to detect accidental copy operations.

Reviewers: rtrieu, rsmith

Reviewed By: rsmith

Subscribers: lebedev.ri, Rakete1111, rsmith, cfe-commits

Tags: #clang

Differential Revision: https://reviews.llvm.org/D43322

Patch by Arthur O'Dwyer.

spurious pushed a commit to spurious/clang-mirror that referenced this issue Apr 12, 2018

malcolm.parsons
Diagnose cases of "return x" that should be "return std::move(x)" for…
… efficiency

Summary:
This patch adds two new diagnostics, which are off by default:

**-Wreturn-std-move**

This diagnostic is enabled by `-Wreturn-std-move`, `-Wmove`, or `-Wall`.
Diagnose cases of `return x` or `throw x`, where `x` is the name of a local variable or parameter, in which a copy operation is performed when a move operation would have been available. The user probably expected a move, but they're not getting a move, perhaps because the type of "x" is different from the return type of the function.
A place where this comes up in the wild is `stdext::inplace_function<Sig, N>` which implements conversion via a conversion operator rather than a converting constructor; see WG21-SG14/SG14#125 (comment)
Another place where this has come up in the wild, but where the fix ended up being different, was

    try { ... } catch (ExceptionType ex) {
        throw ex;
    }

where the appropriate fix in that case was to replace `throw ex;` with `throw;`, and incidentally to catch by reference instead of by value. (But one could contrive a scenario where the slicing was intentional, in which case throw-by-move would have been the appropriate fix after all.)
Another example (intentional slicing to a base class) is dissected in https://github.com/accuBayArea/Slides/blob/master/slides/2018-03-07.pdf

**-Wreturn-std-move-in-c++11**

This diagnostic is enabled only by the exact spelling `-Wreturn-std-move-in-c++11`.
Diagnose cases of "return x;" or "throw x;" which in this version of Clang *do* produce moves, but which prior to Clang 3.9 / GCC 5.1 produced copies instead. This is useful in codebases which care about portability to those older compilers.
The name "-in-c++11" is not technically correct; what caused the version-to-version change in behavior here was actually CWG 1579, not C++14. I think it's likely that codebases that need portability to GCC 4.9-and-earlier may understand "C++11" as a colloquialism for "older compilers." The wording of this diagnostic is based on feedback from @rsmith.

**Discussion**

Notice that this patch is kind of a negative-space version of Richard Trieu's `-Wpessimizing-move`. That diagnostic warns about cases of `return std::move(x)` that should be `return x` for speed. These diagnostics warn about cases of `return x` that should be `return std::move(x)` for speed. (The two diagnostics' bailiwicks do not overlap: we don't have to worry about a `return` statement flipping between the two states indefinitely.)

I propose to write a paper for San Diego that would relax the implicit-move rules so that in C++2a the user //would// see the moves they expect, and the diagnostic could be re-worded in a later version of Clang to suggest explicit `std::move` only "in C++17 and earlier." But in the meantime (and/or forever if that proposal is not well received), this diagnostic will be useful to detect accidental copy operations.

Reviewers: rtrieu, rsmith

Reviewed By: rsmith

Subscribers: lebedev.ri, Rakete1111, rsmith, cfe-commits

Tags: #clang

Differential Revision: https://reviews.llvm.org/D43322

Patch by Arthur O'Dwyer.

git-svn-id: http://llvm.org/svn/llvm-project/cfe/trunk@329914 91177308-0d34-0410-b5e6-96231b3b80d8

arthaud added a commit to NASA-SW-VnV/ikos that referenced this issue Dec 10, 2018

[core] Use explicit std::move to avoid copies
This is reported by clang `-Wreturn-std-move-in-c++11`
See WG21-SG14/SG14#125
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment