Re: [PATCH Coroutines 1/2] Add error messages for missing methods of awaitable class
在 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 * 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 * 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 * 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 * 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
Re: [PATCH Coroutines 1/2] Add error messages for missing methods of awaitable class
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 * 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 * 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 -- Nathan Sidwell
Re: [PATCH Coroutines 1/2] Add error messages for missing methods of awaitable class
在 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 * 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 * 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