Skip to content
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

[BUG] No implicit move for move this, explicit move doesn't parse #857

Closed
JohelEGP opened this issue Nov 26, 2023 · 6 comments · Fixed by #887
Closed

[BUG] No implicit move for move this, explicit move doesn't parse #857

JohelEGP opened this issue Nov 26, 2023 · 6 comments · Fixed by #887
Labels
bug Something isn't working

Comments

@JohelEGP
Copy link
Contributor

Title: No implicit move for move this, explicit move doesn't parse.

Minimal reproducer (https://cpp2.godbolt.org/z/WhxKGfn83):

t: @basic_value type = {
  s: std::string = ();
  private get: (forward self) -> forward _ = self.s;
  operator*: (this) -> forward std::string = get(this);
  operator*: (move this) -> std::string = get(this); // `get(move this)` doesn't parse.
}
main: () = { }
Commands:
cppfront main.cpp2
clang++18 -std=c++23 -stdlib=libc++ -lc++abi -pedantic-errors -Wall -Wextra -Wconversion -Werror=unused-result -Werror=unused-value -Werror=unused-parameter -I . main.cpp

Expected result:

  [[nodiscard]] auto t::operator*() && -> std::string { return get(std::move(*this));  }

Actual result and error:

A copy.

  [[nodiscard]] auto t::operator*() && -> std::string { return get((*this));  }
Cpp2 lowered to Cpp1:
//=== Cpp2 type declarations ====================================================


#include "cpp2util.h"

#line 1 "/app/example.cpp2"
class t;
#line 2 "/app/example.cpp2"
  

//=== Cpp2 type definitions and function declarations ===========================

#line 1 "/app/example.cpp2"
class t {
#line 2 "/app/example.cpp2"
  private: std::string s {}; 
  private: [[nodiscard]] static auto get(auto&& self) -> auto&&;
  public: [[nodiscard]] auto operator*() const& -> std::string const&;
  public: [[nodiscard]] auto operator*() && -> std::string;
  public: t(t const& that);

public: auto operator=(t const& that) -> t& ;
public: t(t&& that) noexcept;
public: auto operator=(t&& that) noexcept -> t& ;
public: explicit t();

#line 6 "/app/example.cpp2"
};
auto main() -> int;

//=== Cpp2 function definitions =================================================

#line 1 "/app/example.cpp2"

#line 3 "/app/example.cpp2"
  [[nodiscard]] auto t::get(auto&& self) -> auto&& { return CPP2_FORWARD(self).s;  }
  [[nodiscard]] auto t::operator*() const& -> std::string const& { return get((*this));  }
  [[nodiscard]] auto t::operator*() && -> std::string { return get((*this));  }

  t::t(t const& that)
                                : s{ that.s }{}

auto t::operator=(t const& that) -> t& {
                                s = that.s;
                                return *this;}
t::t(t&& that) noexcept
                                : s{ std::move(that).s }{}
auto t::operator=(t&& that) noexcept -> t& {
                                s = std::move(that).s;
                                return *this;}
t::t(){}
#line 5 "/app/example.cpp2"
                                                     // `get(move this)` doesn't parse.

#line 7 "/app/example.cpp2"
auto main() -> int{}
Output:
Program returned: 0

See also:

@JohelEGP JohelEGP added the bug Something isn't working label Nov 26, 2023
@JohelEGP
Copy link
Contributor Author

JohelEGP commented Nov 26, 2023

This does parse (https://cpp2.godbolt.org/z/3zdhbPnMo):

operator*: (move this) -> std::string = { return get(move this); }

I have other report(s) where f: () x; doesn't quite behave like f: () -> _ { return x; }:

@JohelEGP
Copy link
Contributor Author

I think it's reasonable to also expect std::move on the implicit this. when accessing a member (https://cpp2.godbolt.org/z/3EnTfjhb6):

  operator*: (move this) -> std::string = s;

Expected:

  [[nodiscard]] auto t::operator*() && -> std::string { return std::move(s);  }

Actual:

  [[nodiscard]] auto t::operator*() && -> std::string { return s;  }

@JohelEGP
Copy link
Contributor Author

Of course, the same should also apply to destructors (https://cpp2.godbolt.org/z/hvxxdaozq):

consume: (copy _: std::unique_ptr<int>) = { }
some_int: type = {
  value: std::unique_ptr<int>;
  operator=: (move this) = {
    consume(value); // Error: Can't copy. Move it!
  }
}
main: () = { }

@JohelEGP
Copy link
Contributor Author

What to do for a data member of reference type?
x, where x is a data member, should lower to std::move(*this).x.
That "moves" x if it doesn't have a reference type.
If x has a reference type, the user has to explicitly move it.
If we moved a data member of reference type on last use,
the opt-out would look something like as-lvalue(x).

@JohelEGP
Copy link
Contributor Author

_ = x; is not a good alternative to prevent the implicit move on last use of a reference member.
Generic code that handles both reference and non-reference data members benefit the most from moving *this and not x.
Otherwise, generic code can't be shared in a move this function that handles both kinds of members.

f: (forward _) = { }
t: @struct <T> type = {
  v: T;
  g: (move this) = { f(v); }      // If `v` lowers to `std::move(*this).x`, this can be correct in both cases.
  h: (move this) = { f(move v); } // If `v` lowers to `std::move(*this).x`, this can be correct in both cases.

  i: (move this) = { f(v); } // If `v` lowers to `std::move(x)`, this can be correct for `T = int`, but not for `T = int&`.

  j: (move this) = { f(v); } // If `v` lowers to `std::move(x)`, this can be correct for `T = int`,
  j: (move this)             // but if it isn't for `T = int&`, this overload is necessary.
    requires std::is_reference_v<T> = {
    f(v);
    _ = v;
  }
}

@JohelEGP
Copy link
Contributor Author

that parameters already work as expected (https://cpp2.godbolt.org/z/4PvTahqb6):

f_copy: (copy _...) = { }
issue_857: @struct type = {
  i: std::unique_ptr<int>;
  f: (move this, move that) = f_copy(/*this,*/ that);
  g: (move this, move that) = f_copy(/*this.i,*/ that.i);
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant