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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

coroutine transformation: Support _promise-constructor-arguments_ #536

Closed
Ukilele opened this issue Jun 2, 2023 · 5 comments
Closed

coroutine transformation: Support _promise-constructor-arguments_ #536

Ukilele opened this issue Jun 2, 2023 · 5 comments
Labels
bug Something isn't working

Comments

@Ukilele
Copy link

Ukilele commented Jun 2, 2023

Hi Andreas,

I am currently learning about coroutines. And even if cppinsights claims to only be an approximation, it is a massive help! Thanks a lot 馃巻

While playing around, I came across a minor issue: The promise-constructor-arguments as described by dcl.fct.def.coroutine#5.7 are not supported by cppinsights. To phrase cppreference:

calls the constructor for the promise object. If the promise type has a constructor that takes all coroutine parameters, that constructor is called, with post-copy coroutine arguments. Otherwise the default constructor is called.

I tried to come up with a minimal example, where the promise_type has a constructor of the form promise_type(int& a, char& b, double& c) and the coroutine (coro) has matching function parameters (int a, char b, double c):

#include <coroutine>
#include <utility>

struct my_resumable {
    struct promise_type;
    using handle_type = std::coroutine_handle<promise_type>;
    my_resumable() = default;
    my_resumable(handle_type h) : m_handle(h) {}
    ~my_resumable() { if (m_handle) m_handle.destroy(); }
    my_resumable(my_resumable&& other) noexcept : m_handle(std::exchange(other.m_handle, nullptr)) {}
    my_resumable& operator=(my_resumable&& other) noexcept {
        m_handle = std::exchange(other.m_handle, nullptr);
        return *this;
    }
    handle_type m_handle;
};

struct my_resumable::promise_type {
    promise_type() = default;
    promise_type(int& a, char& b, double& c) { }

    my_resumable get_return_object() {
        auto h = my_resumable::handle_type::from_promise(*this);
        return my_resumable(h);
    }

    std::suspend_never initial_suspend() { return {}; }
    std::suspend_always final_suspend() noexcept { return {}; }
    void return_void() {}

    void unhandled_exception() {}
};

my_resumable coro(int a, char b, double c) {
    co_return;
}

If you run it through cppinsights, you will see that in line 103/104 the promise object gets constructed like this:

new (&__f->__promise)std::__coroutine_traits_impl<my_resumable>::promise_type{};

But instead it should be:

new (&__f->__promise)std::__coroutine_traits_impl<my_resumable>::promise_type{__f->a, __f->b, __f->c};
@andreasfertig andreasfertig added the bug Something isn't working label Jun 5, 2023
@andreasfertig
Copy link
Owner

Hello @Ukilele,

I am currently learning about coroutines. And even if cppinsights claims to only be an approximation, it is a massive help! Thanks a lot 馃巻

my pleasure!

What you report here is a very interesting case. What I struggle with is this part from your cppreference quote:

with post-copy coroutine arguments

my patch would make it look like what you're showing above. However, I wonder whether another interpretation is:

new (&__f->__promise)std::__coroutine_traits_sfinae<my_resumable>::promise_type{a, b, c};

This form would be broken for moved-from objects, so I assume your version is the correct one.

Andreas

@Ukilele
Copy link
Author

Ukilele commented Jun 6, 2023

Yes, I am pretty sure it is __f->a and not a. This gets also confirmed by https://lewissbaker.github.io/2022/08/27/understanding-the-compiler-transform. The exception to that rule is the *this object in the case that the coroutine is a non-static member function. See: dcl.fct.def.coroutine#4: If the coroutine is a non-static member function, then "q1 is an lvalue that denotes *this".

A minor nitpick: According to dcl.fct.def.coroutine#5.7, it should be (__f->a, __f->b, __f->c), so using parentheses instead of braces. But I don't know if there can be any observable difference between these two.

Thanks,
Kilian

@andreasfertig
Copy link
Owner

Hello @Ukilele,

Yes, I am pretty sure it is __f->a and not a.

perfect.

The exception to that rule is the *this object in the case that the coroutine is a non-static member function. See: dcl.fct.def.coroutine#4: If the coroutine is a non-static member function, then "q1 is an lvalue that denotes *this".

That is an interesting exception. I'm not sure whether C++ Insights currently shows such a case correctly. Do you happen to have an example for this case?

Andreas

@Ukilele
Copy link
Author

Ukilele commented Jun 6, 2023

Here is a snippet, where we have a class ClassWithCoro and

  1. The class ClassWithCoro has a member function coro which is a coroutine (line 26) and
  2. The promise_type has a constructor (line 8) which takes a const ClassWithCoro& q1 (the lvalue that denotes *this) and a int& q2 (the remaining post-copy coroutine arguments).
#include <coroutine>
#include <utility>

struct ClassWithCoro;

struct my_resumable {
  struct promise_type {
    promise_type(const ClassWithCoro& q1, int& q2) {}
    my_resumable get_return_object() {
      return my_resumable(my_resumable::handle_type::from_promise(*this));
    }
    std::suspend_never initial_suspend() { return {}; }
    std::suspend_always final_suspend() noexcept { return {}; }
    void return_void() {}
    void unhandled_exception() {}
  };
  
  using handle_type = std::coroutine_handle<promise_type>;
  my_resumable(handle_type h) : m_handle(h) {}
  ~my_resumable() { if (m_handle) m_handle.destroy(); }
  my_resumable(my_resumable&& other) = delete;
  handle_type m_handle;
};

struct ClassWithCoro {
  my_resumable coro(int x) const {
    co_return;
  }
};

Like I said earlier, I am still learning about coroutines. But I would say that the transformation looks good to me, except of the issue at hand. I.e. that cppinsights generates

new (&__f->__promise)std::__coroutine_traits_impl<my_resumable>::promise_type{};

whereas instead it should generate:

new (&__f->__promise)std::__coroutine_traits_impl<my_resumable>::promise_type(*this, __f->x);

I hope my example is helpful :)

andreasfertig added a commit that referenced this issue Jun 9, 2023
Along with the mentioned fix, this patch fixes two other issues:

1) The return statement in the coroutine setup function was missing the
   `__f`. This error was introduced with the update to Clang 15.
2) The template parameters were missing in the case of a `CXXConversionDecl`
   where the conversion type was a template.
andreasfertig added a commit that referenced this issue Jun 9, 2023
Along with the mentioned fix, this patch fixes two other issues:

1) The return statement in the coroutine setup function was missing the
   `__f`. This error was introduced with the update to Clang 15.
2) The template parameters were missing in the case of a `CXXConversionDecl`
   where the conversion type was a template.
andreasfertig added a commit that referenced this issue Jun 9, 2023
Fixed #536: Call promise constructor if arguments match.
@Ukilele
Copy link
Author

Ukilele commented Jun 16, 2023

Thank you! 馃殌 馃檪

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

No branches or pull requests

2 participants