Skip to content

[Clang] Deducing this does not work correctly with move-only types. #100341

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

Closed
slavek-kucera opened this issue Jul 24, 2024 · 4 comments · Fixed by #120223
Closed

[Clang] Deducing this does not work correctly with move-only types. #100341

slavek-kucera opened this issue Jul 24, 2024 · 4 comments · Fixed by #120223
Labels
clang:frontend Language frontend issues, e.g. anything involving "Sema"

Comments

@slavek-kucera
Copy link

Move only types have to be explicitly cast to r-value for the move constructor to be used. https://godbolt.org/z/fabdhKKEj

#include <utility>

struct X {
    X() = default;
    X(X&&) = default;
    void operator()(this X);
};

void fail() {
    X()();
    [x = X{}](this auto) {}();
}
void pass() {
    std::move(X())();
    std::move([x = X{}](this auto) {})();
}
<source>:10:5: error: call to implicitly-deleted copy constructor of 'X'
   10 |     X()();
      |     ^~~
<source>:5:5: note: copy constructor is implicitly deleted because 'X' has a user-declared move constructor
    5 |     X(X&&) = default;
      |     ^
<source>:6:27: note: passing argument to parameter here
    6 |     void operator()(this X);
      |                           ^
<source>:11:5: error: call to implicitly-deleted copy constructor of '(lambda at <source>:11:5)'
   11 |     [x = X{}](this auto) {}();
      |     ^~~~~~~~~~~~~~~~~~~~~~~
<source>:11:6: note: copy constructor of '(lambda at <source>:11:5)' is implicitly deleted because field '' has a deleted copy constructor
   11 |     [x = X{}](this auto) {}();
      |      ^
<source>:5:5: note: copy constructor is implicitly deleted because 'X' has a user-declared move constructor
    5 |     X(X&&) = default;
      |     ^
<source>:11:24: note: passing argument to parameter here
   11 |     [x = X{}](this auto) {}();
      |                        ^
@github-actions github-actions bot added the clang Clang issues not falling into any other category label Jul 24, 2024
@EugeneZelenko EugeneZelenko added clang:frontend Language frontend issues, e.g. anything involving "Sema" and removed clang Clang issues not falling into any other category labels Jul 24, 2024
@llvmbot
Copy link
Member

llvmbot commented Jul 24, 2024

@llvm/issue-subscribers-clang-frontend

Author: None (slavek-kucera)

Move only types have to be explicitly cast to r-value for the move constructor to be used. https://godbolt.org/z/fabdhKKEj
#include &lt;utility&gt;

struct X {
    X() = default;
    X(X&amp;&amp;) = default;
    void operator()(this X);
};

void fail() {
    X()();
    [x = X{}](this auto) {}();
}
void pass() {
    std::move(X())();
    std::move([x = X{}](this auto) {})();
}
&lt;source&gt;:10:5: error: call to implicitly-deleted copy constructor of 'X'
   10 |     X()();
      |     ^~~
&lt;source&gt;:5:5: note: copy constructor is implicitly deleted because 'X' has a user-declared move constructor
    5 |     X(X&amp;&amp;) = default;
      |     ^
&lt;source&gt;:6:27: note: passing argument to parameter here
    6 |     void operator()(this X);
      |                           ^
&lt;source&gt;:11:5: error: call to implicitly-deleted copy constructor of '(lambda at &lt;source&gt;:11:5)'
   11 |     [x = X{}](this auto) {}();
      |     ^~~~~~~~~~~~~~~~~~~~~~~
&lt;source&gt;:11:6: note: copy constructor of '(lambda at &lt;source&gt;:11:5)' is implicitly deleted because field '' has a deleted copy constructor
   11 |     [x = X{}](this auto) {}();
      |      ^
&lt;source&gt;:5:5: note: copy constructor is implicitly deleted because 'X' has a user-declared move constructor
    5 |     X(X&amp;&amp;) = default;
      |     ^
&lt;source&gt;:11:24: note: passing argument to parameter here
   11 |     [x = X{}](this auto) {}();
      |                        ^

@cor3ntin
Copy link
Contributor

possible duplicate of #100314 @MitalAshok

@MitalAshok
Copy link
Contributor

I don't think this is a dupe, since even without CWG2813 this should have been accepted.
X{}() should have done a temporary materialization conversion and the object parameter should be constructed by moving from the temporary.
It is seemingly coincidentally fixed by allowing the prvalue to bind directly, but there seems to be an existing issue with either the temporary materialization conversion or how those temporary are passed to object parameters. I'll do some investigating.

@MitalAshok
Copy link
Contributor

Problem was here:

!Fun->getParamDecl(0)->getType()->isRValueReferenceType());

The temporary was treated as an lvalue if the explicit object parameter was by-value or an lvalue reference type, and is only an xvalue if it had an rvalue reference type (including void f(this int&&), which is pretty strange). This materialization is removed entirely in #95112. A smaller fix that can easily be cherry picked for Clang 19 is to remove that one line (i.e., the temporary is always an xvalue)

searlmc1 pushed a commit to ROCm/llvm-project that referenced this issue Dec 18, 2024
…0223)

This is a rebase of llvm#95112 with my own feedback apply as @MitalAshok has
been inactive for a while.
It's fairly important this makes clang 20 as it is a blocker for llvm#107451

Change-Id: I2261810f7c8d7dd8b3e3412c0814a528d7c7b91c

---

[CWG2813](https://cplusplus.github.io/CWG/issues/2813.html)

prvalue.member_fn(expression-list) now will not materialize a temporary
for prvalue if member_fn is an explicit object member function, and
prvalue will bind directly to the object parameter.

The E1 in E1.static_member is now a discarded-value expression, so if E1
was a call to a [[nodiscard]] function, there will now be a warning.
This also affects C++98 with [[gnu::warn_unused_result]] functions.

This should not affect C where TemporaryMaterializationConversion is a
no-op.

Closes llvm#100314
Fixes llvm#100341

---------

Co-authored-by: Mital Ashok <[email protected]>
(cherry picked from commit db93ef1)

Change-Id: Ic277d16bc8611b9d383cb890da3eda0ef1646555
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:frontend Language frontend issues, e.g. anything involving "Sema"
Projects
None yet
5 participants