[Coroutines] Add error messages for missing methods of awaitable class

Hi
This patch adds lookup_awaitable_member, it outputs error messages when 
any of
the await_ready/suspend/resume functions are missing in awaitable class.

This patch also add some error check on return value of build_co_await 
since we
may failed to build co_await_expr.

Bootstrap and test on X86_64, is it OK?

Regards
JunMa

gcc/cp
2020-01-20  Jun Ma <JunMa@linux.alibaba.com>

         * coroutines.cc (lookup_awaitable_member): Lookup an awaitable 
member.
         (build_co_await): Use lookup_awaitable_member instead of 
lookup_member.
         (finish_co_yield_expr, finish_co_await_expr): Add error check 
on return
         value of build_co_await.

gcc/testsuite
2020-01-20  Jun Ma <JunMa@linux.alibaba.com>

         * g++.dg/coroutines/coro1-missing-await-method.C: New test.
From 3979b29dcdb1000bbc819f69c1dc3ce7616f87cd Mon Sep 17 00:00:00 2001
From: "liangbin.mj" <liangbin.mj@alibaba-inc.com>
Date: Thu, 21 Nov 2019 08:51:22 +0800
Subject: [PATCH] to #23584419 Add some error messages when can not find method
 of awaitable class.

---
 gcc/cp/coroutines.cc                          | 49 ++++++++++++-------
 .../coroutines/coro1-missing-await-method.C   | 21 ++++++++
 .../coroutines/coro1-ret-int-yield-int.h      |  9 ++++
 3 files changed, 61 insertions(+), 18 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/coroutines/coro1-missing-await-method.C

Hi JunMa,

JunMa <JunMa@linux.alibaba.com> wrote:

> gcc/cp
> 2020-01-20  Jun Ma <JunMa@linux.alibaba.com>
>
>         * coroutines.cc (lookup_awaitable_member): Lookup an awaitable member.
>         (build_co_await): Use lookup_awaitable_member instead of lookup_member.
>         (finish_co_yield_expr, finish_co_await_expr): Add error check on return
>         value of build_co_await.
>
> gcc/testsuite
> 2020-01-20  Jun Ma <JunMa@linux.alibaba.com>
>
>         * g++.dg/coroutines/coro1-missing-await-method.C: New test.
> <0001-Add-some-error-messages-when-missing.patch>

this LGTM, but you will have to wait for a C++ maintainer to approve.
thanks
Iain

在 2020/1/20 下午4:55, Iain Sandoe 写道:
> Hi JunMa,
>
> JunMa <JunMa@linux.alibaba.com> wrote:
>
>> gcc/cp
>> 2020-01-20  Jun Ma <JunMa@linux.alibaba.com>
>>
>>         * coroutines.cc (lookup_awaitable_member): Lookup an 
>> awaitable member.
>>         (build_co_await): Use lookup_awaitable_member instead of 
>> lookup_member.
>>         (finish_co_yield_expr, finish_co_await_expr): Add error check 
>> on return
>>         value of build_co_await.
>>
>> gcc/testsuite
>> 2020-01-20  Jun Ma <JunMa@linux.alibaba.com>
>>
>>         * g++.dg/coroutines/coro1-missing-await-method.C: New test.
>> <0001-Add-some-error-messages-when-missing.patch>
>
> this LGTM, but you will have to wait for a C++ maintainer to approve.
> thanks
> Iain
Thanks Iain,

+Jason and Nathan
could you have a look?

Regards
JunMa

On 1/20/20 12:18 AM, JunMa wrote:
> Hi
> This patch adds lookup_awaitable_member, it outputs error messages when 
> any of
> the await_ready/suspend/resume functions are missing in awaitable class.
> 
> This patch also add some error check on return value of build_co_await 
> since we
> may failed to build co_await_expr.
> 
> Bootstrap and test on X86_64, is it OK?
> 
> Regards
> JunMa
> 
> gcc/cp
> 2020-01-20  Jun Ma <JunMa@linux.alibaba.com>
> 
>          * coroutines.cc (lookup_awaitable_member): Lookup an awaitable 
> member.
>          (build_co_await): Use lookup_awaitable_member instead of 
> lookup_member.
>          (finish_co_yield_expr, finish_co_await_expr): Add error check 
> on return
>          value of build_co_await.
> 
> gcc/testsuite
> 2020-01-20  Jun Ma <JunMa@linux.alibaba.com>
> 
>          * g++.dg/coroutines/coro1-missing-await-method.C: New test.


1) you're mixing two distinct changes, the one about the error message 
and the one about changing error_mark_node.

>    tree op = build_co_await (kw, a, CO_AWAIT_SUSPEND_POINT);
> -  TREE_SIDE_EFFECTS (op) = 1;
> -  SET_EXPR_LOCATION (op, kw);
> -
> +  if (op != error_mark_node)
> +    {
> +      TREE_SIDE_EFFECTS (op) = 1;
> +      SET_EXPR_LOCATION (op, kw);
> +    }
>    return op;
>  }

these two fragments are ok, as a separate patch.


> +/* Lookup an Awaitable member, which should be await_ready, await_suspend
> +   or await_resume  */
> +
> +static tree
> +lookup_awaitable_member (tree await_type, tree member_id, location_t loc)
> +{
> +  tree aw_memb
> +    = lookup_member (await_type, member_id,
> +		     /*protect*/ 1, /*want_type*/ 0, tf_warning_or_error);
fwiw '/*foo=*/expr' is the canonical form -- i know Iain wasn't 
consistent, and it looks like I may have missed a few places in review.

> +  if (aw_memb == NULL_TREE || aw_memb == error_mark_node)
> +    {
> +      error_at (loc, "no member named %qE in %qT", member_id, await_type);
> +      return error_mark_node;
> +    }
> +  return aw_memb;
> +}

This looks wrong.  lookup_member is being passed tf_warning_or_error, so 
it should already be emitting a diagnostic, for the cases where 
something is found, but is ambiguous or whatever.  So, I think you only 
want a diagnostic here when you get NULL_TREE back.

nathan

在 2020/1/20 下午11:49, Nathan Sidwell 写道:
> On 1/20/20 12:18 AM, JunMa wrote:
>> Hi
>> This patch adds lookup_awaitable_member, it outputs error messages 
>> when any of
>> the await_ready/suspend/resume functions are missing in awaitable class.
>>
>> This patch also add some error check on return value of 
>> build_co_await since we
>> may failed to build co_await_expr.
>>
>> Bootstrap and test on X86_64, is it OK?
>>
>> Regards
>> JunMa
>>
>> gcc/cp
>> 2020-01-20  Jun Ma <JunMa@linux.alibaba.com>
>>
>>          * coroutines.cc (lookup_awaitable_member): Lookup an 
>> awaitable member.
>>          (build_co_await): Use lookup_awaitable_member instead of 
>> lookup_member.
>>          (finish_co_yield_expr, finish_co_await_expr): Add error 
>> check on return
>>          value of build_co_await.
>>
>> gcc/testsuite
>> 2020-01-20  Jun Ma <JunMa@linux.alibaba.com>
>>
>>          * g++.dg/coroutines/coro1-missing-await-method.C: New test.
>
>
> 1) you're mixing two distinct changes, the one about the error message 
> and the one about changing error_mark_node.
>
>>    tree op = build_co_await (kw, a, CO_AWAIT_SUSPEND_POINT);
>> -  TREE_SIDE_EFFECTS (op) = 1;
>> -  SET_EXPR_LOCATION (op, kw);
>> -
>> +  if (op != error_mark_node)
>> +    {
>> +      TREE_SIDE_EFFECTS (op) = 1;
>> +      SET_EXPR_LOCATION (op, kw);
>> +    }
>>    return op;
>>  }
>
> these two fragments are ok, as a separate patch.
>
>
ok, I'll split it.
>> +/* Lookup an Awaitable member, which should be await_ready, 
>> await_suspend
>> +   or await_resume  */
>> +
>> +static tree
>> +lookup_awaitable_member (tree await_type, tree member_id, location_t 
>> loc)
>> +{
>> +  tree aw_memb
>> +    = lookup_member (await_type, member_id,
>> +             /*protect*/ 1, /*want_type*/ 0, tf_warning_or_error);
> fwiw '/*foo=*/expr' is the canonical form -- i know Iain wasn't 
> consistent, and it looks like I may have missed a few places in review.
>
ok.
>> +  if (aw_memb == NULL_TREE || aw_memb == error_mark_node)
>> +    {
>> +      error_at (loc, "no member named %qE in %qT", member_id, 
>> await_type);
>> +      return error_mark_node;
>> +    }
>> +  return aw_memb;
>> +}
>
> This looks wrong.  lookup_member is being passed tf_warning_or_error, 
> so it should already be emitting a diagnostic, for the cases where 
> something is found, but is ambiguous or whatever.  So, I think you 
> only want a diagnostic here when you get NULL_TREE back.
>
you are right, I follow with same code of lookup_promise_member, I'll 
update both.

Regards
JunMa
> nathan
>

在 2020/1/21 上午9:31, JunMa 写道:
> 在 2020/1/20 下午11:49, Nathan Sidwell 写道:
>> On 1/20/20 12:18 AM, JunMa wrote:
>>> Hi
>>> This patch adds lookup_awaitable_member, it outputs error messages 
>>> when any of
>>> the await_ready/suspend/resume functions are missing in awaitable 
>>> class.
>>>
>>> This patch also add some error check on return value of 
>>> build_co_await since we
>>> may failed to build co_await_expr.
>>>
>>> Bootstrap and test on X86_64, is it OK?
>>>
>>> Regards
>>> JunMa
>>>
>>> gcc/cp
>>> 2020-01-20  Jun Ma <JunMa@linux.alibaba.com>
>>>
>>>          * coroutines.cc (lookup_awaitable_member): Lookup an 
>>> awaitable member.
>>>          (build_co_await): Use lookup_awaitable_member instead of 
>>> lookup_member.
>>>          (finish_co_yield_expr, finish_co_await_expr): Add error 
>>> check on return
>>>          value of build_co_await.
>>>
>>> gcc/testsuite
>>> 2020-01-20  Jun Ma <JunMa@linux.alibaba.com>
>>>
>>>          * g++.dg/coroutines/coro1-missing-await-method.C: New test.
>>
>>
>> 1) you're mixing two distinct changes, the one about the error 
>> message and the one about changing error_mark_node.
>>
>>>    tree op = build_co_await (kw, a, CO_AWAIT_SUSPEND_POINT);
>>> -  TREE_SIDE_EFFECTS (op) = 1;
>>> -  SET_EXPR_LOCATION (op, kw);
>>> -
>>> +  if (op != error_mark_node)
>>> +    {
>>> +      TREE_SIDE_EFFECTS (op) = 1;
>>> +      SET_EXPR_LOCATION (op, kw);
>>> +    }
>>>    return op;
>>>  }
>>
>> these two fragments are ok, as a separate patch.
>>
>>
> ok, I'll split it.
>>> +/* Lookup an Awaitable member, which should be await_ready, 
>>> await_suspend
>>> +   or await_resume  */
>>> +
>>> +static tree
>>> +lookup_awaitable_member (tree await_type, tree member_id, 
>>> location_t loc)
>>> +{
>>> +  tree aw_memb
>>> +    = lookup_member (await_type, member_id,
>>> +             /*protect*/ 1, /*want_type*/ 0, tf_warning_or_error);
>> fwiw '/*foo=*/expr' is the canonical form -- i know Iain wasn't 
>> consistent, and it looks like I may have missed a few places in review.
>>
> ok.
>>> +  if (aw_memb == NULL_TREE || aw_memb == error_mark_node)
>>> +    {
>>> +      error_at (loc, "no member named %qE in %qT", member_id, 
>>> await_type);
>>> +      return error_mark_node;
>>> +    }
>>> +  return aw_memb;
>>> +}
>>
>> This looks wrong.  lookup_member is being passed tf_warning_or_error, 
>> so it should already be emitting a diagnostic, for the cases where 
>> something is found, but is ambiguous or whatever.  So, I think you 
>> only want a diagnostic here when you get NULL_TREE back.
>>
> you are right, I follow with same code of lookup_promise_member, I'll 
> update both.
>
> Regards
> JunMa
>> nathan
>>
Hi nathan,
attachment is the updated patch which add error messages for lookup 
awaitable member.

Regards
JunMa

gcc/cp
2020-01-21  Jun Ma <JunMa@linux.alibaba.com>

          * coroutines.cc (lookup_awaitable_member): Lookup an awaitable 
member.
          (lookup_promise_method): Emit diagnostic when get NULL_TREE 
back only.
          (build_co_await): Use lookup_awaitable_member instead of 
lookup_member.

gcc/testsuite
2020-01-21  Jun Ma <JunMa@linux.alibaba.com>

          * g++.dg/coroutines/coro1-missing-await-method.C: New test.
---
 gcc/cp/coroutines.cc                          | 36 ++++++++++++-------
 .../coroutines/coro1-missing-await-method.C   | 21 +++++++++++
 .../coroutines/coro1-ret-int-yield-int.h      |  9 +++++
 3 files changed, 53 insertions(+), 13 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/coroutines/coro1-missing-await-method.C

diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc
index d3aacd7ad3b..43f03f7c49a 100644
--- a/gcc/cp/coroutines.cc
+++ b/gcc/cp/coroutines.cc
@@ -496,8 +496,8 @@ lookup_promise_method (tree fndecl, tree member_id, location_t loc,
   tree promise = get_coroutine_promise_type (fndecl);
   tree pm_memb
     = lookup_member (promise, member_id,
-		     /*protect*/ 1, /*want_type*/ 0, tf_warning_or_error);
-  if (musthave && (pm_memb == NULL_TREE || pm_memb == error_mark_node))
+		     /*protect=*/ 1, /*want_type=*/ 0, tf_warning_or_error);
+  if (musthave && pm_memb == NULL_TREE)
     {
       error_at (loc, "no member named %qE in %qT", member_id, promise);
       return error_mark_node;
@@ -505,6 +505,23 @@ lookup_promise_method (tree fndecl, tree member_id, location_t loc,
   return pm_memb;
 }
 
+/* Lookup an Awaitable member, which should be await_ready, await_suspend
+   or await_resume.  */
+
+static tree
+lookup_awaitable_member (tree await_type, tree member_id, location_t loc)
+{
+  tree aw_memb
+    = lookup_member (await_type, member_id,
+		     /*protect=*/ 1, /*want_type=*/ 0, tf_warning_or_error);
+  if (aw_memb == NULL_TREE)
+    {
+      error_at (loc, "no member named %qE in %qT", member_id, await_type);
+      return error_mark_node;
+    }
+  return aw_memb;
+}
+
 /* Here we check the constraints that are common to all keywords (since the
    presence of a coroutine keyword makes the function into a coroutine).  */
 
@@ -643,25 +660,18 @@ build_co_await (location_t loc, tree a, suspend_point_kind suspend_kind)
 
   /* Check for required awaitable members and their types.  */
   tree awrd_meth
-    = lookup_member (o_type, coro_await_ready_identifier,
-		     /* protect */ 1, /*want_type=*/0, tf_warning_or_error);
-
+    = lookup_awaitable_member (o_type, coro_await_ready_identifier, loc);
   if (!awrd_meth || awrd_meth == error_mark_node)
     return error_mark_node;
-
   tree awsp_meth
-    = lookup_member (o_type, coro_await_suspend_identifier,
-		     /* protect */ 1, /*want_type=*/0, tf_warning_or_error);
-
+    = lookup_awaitable_member (o_type, coro_await_suspend_identifier, loc);
   if (!awsp_meth || awsp_meth == error_mark_node)
     return error_mark_node;
 
   /* The type of the co_await is the return type of the awaitable's
-     co_resume(), so we need to look that up.  */
+     await_resume, so we need to look that up.  */
   tree awrs_meth
-    = lookup_member (o_type, coro_await_resume_identifier,
-		     /* protect */ 1, /*want_type=*/0, tf_warning_or_error);
-
+    = lookup_awaitable_member (o_type, coro_await_resume_identifier, loc);
   if (!awrs_meth || awrs_meth == error_mark_node)
     return error_mark_node;
 
diff --git a/gcc/testsuite/g++.dg/coroutines/coro1-missing-await-method.C b/gcc/testsuite/g++.dg/coroutines/coro1-missing-await-method.C
new file mode 100644
index 00000000000..c1869e0654c
--- /dev/null
+++ b/gcc/testsuite/g++.dg/coroutines/coro1-missing-await-method.C
@@ -0,0 +1,21 @@
+//  { dg-additional-options "-fsyntax-only -w" }
+#include "coro.h"
+
+#define MISSING_AWAIT_READY
+#define MISSING_AWAIT_SUSPEND
+#define MISSING_AWAIT_RESUME
+#include "coro1-ret-int-yield-int.h"
+
+coro1
+bar0 () // { dg-error {no member named 'await_suspend' in 'coro1::suspend_always_prt'} }
+{
+  co_await coro1::suspend_never_prt{}; // { dg-error {no member named 'await_ready' in 'coro1::suspend_never_prt'} }
+  co_yield 5; // { dg-error {no member named 'await_suspend' in 'coro1::suspend_always_prt'} }
+  co_await coro1::suspend_always_intprt(5); // { dg-error {no member named 'await_resume' in 'coro1::suspend_always_intprt'} }
+  co_return 0;
+}
+
+int main (int ac, char *av[]) {
+  struct coro1 x0 = bar0 ();
+  return 0;
+}
diff --git a/gcc/testsuite/g++.dg/coroutines/coro1-ret-int-yield-int.h b/gcc/testsuite/g++.dg/coroutines/coro1-ret-int-yield-int.h
index b961755e472..abf625869fa 100644
--- a/gcc/testsuite/g++.dg/coroutines/coro1-ret-int-yield-int.h
+++ b/gcc/testsuite/g++.dg/coroutines/coro1-ret-int-yield-int.h
@@ -27,14 +27,20 @@ struct coro1 {
   // Some awaitables to use in tests.
   // With progress printing for debug.
   struct suspend_never_prt {
+#ifdef MISSING_AWAIT_READY
+#else
   bool await_ready() const noexcept { return true; }
+#endif
   void await_suspend(handle_type) const noexcept { PRINT ("susp-never-susp");}
   void await_resume() const noexcept { PRINT ("susp-never-resume");}
   };
 
   struct  suspend_always_prt {
   bool await_ready() const noexcept { return false; }
+#ifdef MISSING_AWAIT_SUSPEND
+#else
   void await_suspend(handle_type) const noexcept { PRINT ("susp-always-susp");}
+#endif
   void await_resume() const noexcept { PRINT ("susp-always-resume");}
   ~suspend_always_prt() { PRINT ("susp-always-dtor"); }
   };
@@ -46,7 +52,10 @@ struct coro1 {
     ~suspend_always_intprt() {}
     bool await_ready() const noexcept { return false; }
     void await_suspend(coro::coroutine_handle<>) const noexcept { PRINT ("susp-always-susp-intprt");}
+#ifdef MISSING_AWAIT_RESUME
+#else
     int await_resume() const noexcept { PRINT ("susp-always-resume-intprt"); return x;}
+#endif
   };
   
   /* This returns the square of the int that it was constructed with.  */

在 2020/1/21 上午9:31, JunMa 写道:
> 在 2020/1/20 下午11:49, Nathan Sidwell 写道:
>> On 1/20/20 12:18 AM, JunMa wrote:
>>> Hi
>>> This patch adds lookup_awaitable_member, it outputs error messages 
>>> when any of
>>> the await_ready/suspend/resume functions are missing in awaitable 
>>> class.
>>>
>>> This patch also add some error check on return value of 
>>> build_co_await since we
>>> may failed to build co_await_expr.
>>>
>>> Bootstrap and test on X86_64, is it OK?
>>>
>>> Regards
>>> JunMa
>>>
>>> gcc/cp
>>> 2020-01-20  Jun Ma <JunMa@linux.alibaba.com>
>>>
>>>          * coroutines.cc (lookup_awaitable_member): Lookup an 
>>> awaitable member.
>>>          (build_co_await): Use lookup_awaitable_member instead of 
>>> lookup_member.
>>>          (finish_co_yield_expr, finish_co_await_expr): Add error 
>>> check on return
>>>          value of build_co_await.
>>>
>>> gcc/testsuite
>>> 2020-01-20  Jun Ma <JunMa@linux.alibaba.com>
>>>
>>>          * g++.dg/coroutines/coro1-missing-await-method.C: New test.
>>
>>
>> 1) you're mixing two distinct changes, the one about the error 
>> message and the one about changing error_mark_node.
>>
>>>    tree op = build_co_await (kw, a, CO_AWAIT_SUSPEND_POINT);
>>> -  TREE_SIDE_EFFECTS (op) = 1;
>>> -  SET_EXPR_LOCATION (op, kw);
>>> -
>>> +  if (op != error_mark_node)
>>> +    {
>>> +      TREE_SIDE_EFFECTS (op) = 1;
>>> +      SET_EXPR_LOCATION (op, kw);
>>> +    }
>>>    return op;
>>>  }
>>
>> these two fragments are ok, as a separate patch.
>>
>>
> ok, I'll split it.
>>> +/* Lookup an Awaitable member, which should be await_ready, 
>>> await_suspend
>>> +   or await_resume  */
>>> +
>>> +static tree
>>> +lookup_awaitable_member (tree await_type, tree member_id, 
>>> location_t loc)
>>> +{
>>> +  tree aw_memb
>>> +    = lookup_member (await_type, member_id,
>>> +             /*protect*/ 1, /*want_type*/ 0, tf_warning_or_error);
>> fwiw '/*foo=*/expr' is the canonical form -- i know Iain wasn't 
>> consistent, and it looks like I may have missed a few places in review.
>>
> ok.
>>> +  if (aw_memb == NULL_TREE || aw_memb == error_mark_node)
>>> +    {
>>> +      error_at (loc, "no member named %qE in %qT", member_id, 
>>> await_type);
>>> +      return error_mark_node;
>>> +    }
>>> +  return aw_memb;
>>> +}
>>
>> This looks wrong.  lookup_member is being passed tf_warning_or_error, 
>> so it should already be emitting a diagnostic, for the cases where 
>> something is found, but is ambiguous or whatever.  So, I think you 
>> only want a diagnostic here when you get NULL_TREE back.
>>
> you are right, I follow with same code of lookup_promise_member, I'll 
> update both.
>
> Regards
> JunMa
>> nathan
>>
Hi nathan,
attachment is the updated patch which check error_mark_node of 
build_co_coawait.

Regards
JunMa

gcc/cp
2020-01-21  Jun Ma <JunMa@linux.alibaba.com>

          * coroutines.cc (finish_co_await_expr): Add error check on return
          value of build_co_await.
          (finish_co_yield_expr,): Ditto.
---
 gcc/cp/coroutines.cc | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc
index 43f03f7c49a..bd3656562ec 100644
--- a/gcc/cp/coroutines.cc
+++ b/gcc/cp/coroutines.cc
@@ -810,8 +810,11 @@ finish_co_await_expr (location_t kw, tree expr)
 
   /* Now we want to build co_await a.  */
   tree op = build_co_await (kw, a, CO_AWAIT_SUSPEND_POINT);
-  TREE_SIDE_EFFECTS (op) = 1;
-  SET_EXPR_LOCATION (op, kw);
+  if (op != error_mark_node)
+    {
+      TREE_SIDE_EFFECTS (op) = 1;
+      SET_EXPR_LOCATION (op, kw);
+    }
 
   return op;
 }
@@ -874,9 +877,11 @@ finish_co_yield_expr (location_t kw, tree expr)
      promise transform_await().  */
 
   tree op = build_co_await (kw, yield_call, CO_YIELD_SUSPEND_POINT);
-
-  op = build2_loc (kw, CO_YIELD_EXPR, TREE_TYPE (op), expr, op);
-  TREE_SIDE_EFFECTS (op) = 1;
+  if (op != error_mark_node)
+    {
+      op = build2_loc (kw, CO_YIELD_EXPR, TREE_TYPE (op), expr, op);
+      TREE_SIDE_EFFECTS (op) = 1;
+    }
 
   return op;
 }

On 1/20/20 10:38 PM, JunMa wrote:
> 在 2020/1/21 上午9:31, JunMa 写道:
>> 在 2020/1/20 下午11:49, Nathan Sidwell 写道:
>>> On 1/20/20 12:18 AM, JunMa wrote:
>>>> Hi
>>>> This patch adds lookup_awaitable_member, it outputs error messages 
>>>> when any of
>>>> the await_ready/suspend/resume functions are missing in awaitable 
>>>> class.
>>>>
>>>> This patch also add some error check on return value of 
>>>> build_co_await since we
>>>> may failed to build co_await_expr.
>>>>
>>>> Bootstrap and test on X86_64, is it OK?
>>>>
>>>> Regards
>>>> JunMa
>>>>
>>>> gcc/cp
>>>> 2020-01-20  Jun Ma <JunMa@linux.alibaba.com>
>>>>
>>>>          * coroutines.cc (lookup_awaitable_member): Lookup an 
>>>> awaitable member.
>>>>          (build_co_await): Use lookup_awaitable_member instead of 
>>>> lookup_member.
>>>>          (finish_co_yield_expr, finish_co_await_expr): Add error 
>>>> check on return
>>>>          value of build_co_await.
>>>>
>>>> gcc/testsuite
>>>> 2020-01-20  Jun Ma <JunMa@linux.alibaba.com>
>>>>
>>>>          * g++.dg/coroutines/coro1-missing-await-method.C: New test.
>>>
>>>
>>> 1) you're mixing two distinct changes, the one about the error 
>>> message and the one about changing error_mark_node.
>>>
>>>>    tree op = build_co_await (kw, a, CO_AWAIT_SUSPEND_POINT);
>>>> -  TREE_SIDE_EFFECTS (op) = 1;
>>>> -  SET_EXPR_LOCATION (op, kw);
>>>> -
>>>> +  if (op != error_mark_node)
>>>> +    {
>>>> +      TREE_SIDE_EFFECTS (op) = 1;
>>>> +      SET_EXPR_LOCATION (op, kw);
>>>> +    }
>>>>    return op;
>>>>  }
>>>
>>> these two fragments are ok, as a separate patch.
>>>
>>>
>> ok, I'll split it.
>>>> +/* Lookup an Awaitable member, which should be await_ready, 
>>>> await_suspend
>>>> +   or await_resume  */
>>>> +
>>>> +static tree
>>>> +lookup_awaitable_member (tree await_type, tree member_id, 
>>>> location_t loc)
>>>> +{
>>>> +  tree aw_memb
>>>> +    = lookup_member (await_type, member_id,
>>>> +             /*protect*/ 1, /*want_type*/ 0, tf_warning_or_error);
>>> fwiw '/*foo=*/expr' is the canonical form -- i know Iain wasn't 
>>> consistent, and it looks like I may have missed a few places in review.
>>>
>> ok.
>>>> +  if (aw_memb == NULL_TREE || aw_memb == error_mark_node)
>>>> +    {
>>>> +      error_at (loc, "no member named %qE in %qT", member_id, 
>>>> await_type);
>>>> +      return error_mark_node;
>>>> +    }
>>>> +  return aw_memb;
>>>> +}
>>>
>>> This looks wrong.  lookup_member is being passed tf_warning_or_error, 
>>> so it should already be emitting a diagnostic, for the cases where 
>>> something is found, but is ambiguous or whatever.  So, I think you 
>>> only want a diagnostic here when you get NULL_TREE back.
>>>
>> you are right, I follow with same code of lookup_promise_member, I'll 
>> update both.
>>
>> Regards
>> JunMa
>>> nathan
>>>
> Hi nathan,
> attachment is the updated patch which add error messages for lookup 
> awaitable member.

thanks this is ok.  Could you remove the space in '/*protect=*/ 1' and 
similar?  This appear to be one place where our coding style has fewer 
spaces than expected!

nathan

On 1/20/20 10:44 PM, JunMa wrote:
> 在 2020/1/21 上午9:31, JunMa 写道:

>> Regards
>> JunMa
>>> nathan
>>>
> Hi nathan,
> attachment is the updated patch which check error_mark_node of 
> build_co_coawait.
> 
> Regards
> JunMa
> 
> gcc/cp
> 2020-01-21  Jun Ma <JunMa@linux.alibaba.com>
> 
>           * coroutines.cc (finish_co_await_expr): Add error check on return
>           value of build_co_await.
>           (finish_co_yield_expr,): Ditto.

ok, thanks

nathan

在 2020/1/21 下午8:06, Nathan Sidwell 写道:
> On 1/20/20 10:38 PM, JunMa wrote:
>> 在 2020/1/21 上午9:31, JunMa 写道:
>>> 在 2020/1/20 下午11:49, Nathan Sidwell 写道:
>>>> On 1/20/20 12:18 AM, JunMa wrote:
>>>>> Hi
>>>>> This patch adds lookup_awaitable_member, it outputs error messages 
>>>>> when any of
>>>>> the await_ready/suspend/resume functions are missing in awaitable 
>>>>> class.
>>>>>
>>>>> This patch also add some error check on return value of 
>>>>> build_co_await since we
>>>>> may failed to build co_await_expr.
>>>>>
>>>>> Bootstrap and test on X86_64, is it OK?
>>>>>
>>>>> Regards
>>>>> JunMa
>>>>>
>>>>> gcc/cp
>>>>> 2020-01-20  Jun Ma <JunMa@linux.alibaba.com>
>>>>>
>>>>>          * coroutines.cc (lookup_awaitable_member): Lookup an 
>>>>> awaitable member.
>>>>>          (build_co_await): Use lookup_awaitable_member instead of 
>>>>> lookup_member.
>>>>>          (finish_co_yield_expr, finish_co_await_expr): Add error 
>>>>> check on return
>>>>>          value of build_co_await.
>>>>>
>>>>> gcc/testsuite
>>>>> 2020-01-20  Jun Ma <JunMa@linux.alibaba.com>
>>>>>
>>>>>          * g++.dg/coroutines/coro1-missing-await-method.C: New test.
>>>>
>>>>
>>>> 1) you're mixing two distinct changes, the one about the error 
>>>> message and the one about changing error_mark_node.
>>>>
>>>>>    tree op = build_co_await (kw, a, CO_AWAIT_SUSPEND_POINT);
>>>>> -  TREE_SIDE_EFFECTS (op) = 1;
>>>>> -  SET_EXPR_LOCATION (op, kw);
>>>>> -
>>>>> +  if (op != error_mark_node)
>>>>> +    {
>>>>> +      TREE_SIDE_EFFECTS (op) = 1;
>>>>> +      SET_EXPR_LOCATION (op, kw);
>>>>> +    }
>>>>>    return op;
>>>>>  }
>>>>
>>>> these two fragments are ok, as a separate patch.
>>>>
>>>>
>>> ok, I'll split it.
>>>>> +/* Lookup an Awaitable member, which should be await_ready, 
>>>>> await_suspend
>>>>> +   or await_resume  */
>>>>> +
>>>>> +static tree
>>>>> +lookup_awaitable_member (tree await_type, tree member_id, 
>>>>> location_t loc)
>>>>> +{
>>>>> +  tree aw_memb
>>>>> +    = lookup_member (await_type, member_id,
>>>>> +             /*protect*/ 1, /*want_type*/ 0, tf_warning_or_error);
>>>> fwiw '/*foo=*/expr' is the canonical form -- i know Iain wasn't 
>>>> consistent, and it looks like I may have missed a few places in 
>>>> review.
>>>>
>>> ok.
>>>>> +  if (aw_memb == NULL_TREE || aw_memb == error_mark_node)
>>>>> +    {
>>>>> +      error_at (loc, "no member named %qE in %qT", member_id, 
>>>>> await_type);
>>>>> +      return error_mark_node;
>>>>> +    }
>>>>> +  return aw_memb;
>>>>> +}
>>>>
>>>> This looks wrong.  lookup_member is being passed 
>>>> tf_warning_or_error, so it should already be emitting a diagnostic, 
>>>> for the cases where something is found, but is ambiguous or 
>>>> whatever.  So, I think you only want a diagnostic here when you get 
>>>> NULL_TREE back.
>>>>
>>> you are right, I follow with same code of lookup_promise_member, 
>>> I'll update both.
>>>
>>> Regards
>>> JunMa
>>>> nathan
>>>>
>> Hi nathan,
>> attachment is the updated patch which add error messages for lookup 
>> awaitable member.
>
> thanks this is ok.  Could you remove the space in '/*protect=*/ 1' and 
> similar?  This appear to be one place where our coding style has fewer 
> spaces than expected!
>
ok, I'll update it.

Regards
JunMa
> nathan
>