From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 21180 invoked by alias); 18 Nov 2019 13:23:18 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 21165 invoked by uid 89); 18 Nov 2019 13:23:18 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-6.9 required=5.0 tests=BAYES_00,FREEMAIL_FROM,GIT_PATCH_3,RCVD_IN_DNSWL_NONE,SPF_PASS autolearn=ham version=3.3.1 spammy=HX-Spam-Relays-External:209.85.222.196, instantiate, H*RU:209.85.222.196 X-HELO: mail-qk1-f196.google.com Received: from mail-qk1-f196.google.com (HELO mail-qk1-f196.google.com) (209.85.222.196) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 18 Nov 2019 13:23:15 +0000 Received: by mail-qk1-f196.google.com with SMTP id e2so14337677qkn.5; Mon, 18 Nov 2019 05:23:15 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=sender:subject:to:references:from:message-id:date:user-agent :mime-version:in-reply-to:content-language:content-transfer-encoding; bh=dX/F68IWsu8I6TAW6UDrDRgn0VG7rO3d0S9tnlVc6lo=; b=lXG0tY4CnVeh6F+2iZwk1rLB94nbyocov9OL+5YWsOTTPrxzMoeT1EDJOGr/NbGjM7 wHGSXnuDvKpHpe1gVikz3gMd0EsPkrZxNPbf0X9aPJXzzaiLRmoqY6t8FF7XUipnqS1c kL4tv66++BWkOuYHRDSX8TicWf/Z9cLYZZrnzsg4+cVQwmlT34oeZxD6UdiHkpReP8mv kNdsWcmmkGn7xyC11LAetex70nd3NzEQVfon4fCWTA2VaicnbNZjDamcm1gF/he/G4NJ P6iu/Ash5oWycJSIvU9tLf8y5YQdi+qRwuBhAXVGugqQC2pNtgz4W/O/njoOEXdzGGwg 9XZw== Return-Path: Received: from ?IPv6:2620:10d:c0a3:1407:f9db:facc:a4cd:69ec? ([2620:10d:c091:500::1:c4d6]) by smtp.googlemail.com with ESMTPSA id i14sm8665275qke.102.2019.11.18.05.23.12 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 18 Nov 2019 05:23:12 -0800 (PST) Subject: Re: [C++ coroutines 3/6] Front end parsing and transforms. To: Iain Sandoe , GCC Patches , libstdc++ References: <285E6AA6-17E6-4E7F-9F37-852707896DA1@sandoe.co.uk> From: Nathan Sidwell Message-ID: Date: Mon, 18 Nov 2019 13:24:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.1.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit X-SW-Source: 2019-11/txt/msg01710.txt.bz2 On 11/17/19 5:25 AM, Iain Sandoe wrote: > +++ b/gcc/cp/coroutines.cc > +/* FIXME: minimise headers.. */ FIXED? > +/* DEBUG remove me. */ > +extern void debug_tree (tree); ? > +static tree find_coro_traits_template_decl (location_t); > +static tree find_coro_handle_type (location_t, tree); > +static tree find_promise_type (tree); > +static tree > +lookup_promise_member (tree, const char *, location_t, bool); bad line break? > +/* Lookup std::experimental. */ > +static tree > +find_std_experimental (location_t loc) Would this be better caching into a global_tree? > +{ > + /* we want std::experimental::coroutine_traits class template decl. */ ... or is the template_decl the thing to cache? (Oh I see this comment is incomplete as there's promise_type too). > +/* Lookup the coroutine_traits template decl. > + Instantiate that for the function signature. */ > + > +static tree > +find_coro_traits_template_decl (location_t kw) > +{ > + > + tree traits_name = get_identifier ("coroutine_traits"); > + tree traits_decl > + = lookup_template_class (traits_name, targ, > + /* in_decl */ NULL_TREE, > + /* context */ exp_ns, > + /* entering scope */ false, tf_none); You should be able to pass the TEMPLATE_DECL into lookup_template_class. So, yes cache the TEMPLATE_DECL std::experimental::coroutine_traits on first lookup into a global tree. > + > + if (traits_decl == error_mark_node) > + { > + error_at (kw, "couldn't instantiate coroutine_traits"); couldn't -> cannot (or something else non-apostrophey) > +static tree > +find_coro_handle_type (location_t kw, tree promise_type) > +{ > + tree exp_ns = find_std_experimental (kw); > + if (!exp_ns) > + return NULL_TREE; > + > + /* So now build up a type list for the template, one entry, the promise. */ > + tree targ = make_tree_vec (1); > + TREE_VEC_ELT (targ, 0) = promise_type; > + tree handle_name = get_identifier ("coroutine_handle"); > + tree handle_type > + = lookup_template_class (handle_name, targ, As with the traits, cache the TEMPLATE_DECL. > +/* Look for the promise_type in the instantiated. */ > + > +static tree > +find_promise_type (tree handle_type) > +{ > + tree promise_name = get_identifier ("promise_type"); could you add these identifiers to cp_global_trees? (use-once identifiers fine not to, but there;s no point continually rehashing multi-use ones). > +/* The state that we collect during parsing (and template expansion) for > + a coroutine. */ > +typedef struct coroutine_info > +{ > + tree promise_type; > + tree handle_type; > + tree self_h_proxy; > + tree promise_proxy; > + location_t first_coro_keyword; > +} coroutine_info_t; typedef struct X{} X_t; is C-like. Please comment the fields though. Doesn't this need GTYing? What keeps those trees alive across GC otherwise? (try running with gc-always and see what happens?) > +/* These function assumes that the caller has verified that the state for function->functions > + the decl has been initialised, we try to minimise work here. */ IIUC american/oxford-english IZED? > +static tree > +get_coroutine_promise_type (tree decl) > +{ > + gcc_checking_assert (fn_to_coro_info); > + > + coroutine_info_t *info = fn_to_coro_info->get (decl); > + if (!info) > + return NULL_TREE; > + return info->promise_type; idiomatic C++ would be if (coroutine_info_t *info = ...) return info->promis_type; return NULL_TREE; your call. > +/* Here we check the constraints that are common to all keywords (since the > + presence of a coroutine keyword makes the function into a coroutine). */ > + > + /* This is arranged in order of prohibitions in the std. */ could you add a [clause.subname] reference maybe? > + if (DECL_MAIN_P (fndecl)) > +/* Here we will check the constraints that are not per keyword. */ will-> "" > + > +static bool > +coro_function_valid_p (tree fndecl) > +{ > + location_t f_loc = DECL_SOURCE_LOCATION (fndecl); > + > + /* Since we think the function is a coroutine, that implies we parsed > + a keyword that triggered this. Keywords check promise validity for > + their context and thus the promise type should be known at this point. > + */ unfortunate line break? > + gcc_assert (get_coroutine_handle_type (fndecl) != NULL_TREE > + && get_coroutine_promise_type (fndecl) != NULL_TREE); > + > + if (current_function_returns_value || current_function_returns_null) > + /* TODO: record or extract positions of returns (and the first coro > + keyword) so that we can add notes to the diagnostic about where > + the bad keyword is and what made the function into a coro. */ > + error_at (f_loc, "return statement not allowed in coroutine;" <%return%> not allowed ...? > +/* This performs [expr.await] bullet 3.3 and validates the interface obtained. > + It is also used to build the initial and final suspend points. > + > + A is the original await expr. > + MODE: > + 0 = regular function body co_await > + 1 = await from a co_yield > + 2 = initial await > + 3 = final await. > +*/ Would MODE be better as an enum, from whence you cons up the INTEGER_CST you need for the calls? I'll bet that makes calling code clearer. MODE probably not a good name, given it's usual meaning of machine_mode. > +static tree > +build_co_await (location_t loc, tree a, tree mode) > +{ > + tree o_type = complete_type_or_else (TREE_TYPE (o), o); > + if (TREE_CODE (o_type) != RECORD_TYPE) > + { > + error_at (loc, > + "member reference base type %qT is not a" > + " structure or union", "member reference base type" sounds confusing to me. > + /* To complete the lookups, we need an instance of 'e' which is built from > + 'o' according to [expr.await] 3.4. However, we don't want to materialise ise->ize? > + /* The suspend method has constraints on its return type. */ hm, as 'constraint' is now a term of art wrt concepts, perhaps 'requirements'? > + bool OK = false; OK ->ok? > + tree susp_return_type = TYPE_CANONICAL (TREE_TYPE (TREE_TYPE (awsp_func))); Why are you looking at TYPE_CANONICAL, that seems fishy. > + if (same_type_p (susp_return_type, void_type_node)) > + OK = true; > + else if (same_type_p (susp_return_type, boolean_type_node)) > + OK = true; > + else if (TREE_CODE (susp_return_type) == RECORD_TYPE) > + /* TODO: this isn't enough of a test. */ What would complete it? > + if (!OK) > + { > + fprintf (stderr, "didn't grok the suspend return : "); > + debug_tree (susp_return_type); what's going on here? > +tree > +finish_co_await_expr (location_t kw, tree expr) > +{ > + > + if (expr == NULL_TREE) > + { > + error_at (kw, "% requires an expression."); isn't this a syntactic restriction? How can we get here without already going wrong? > + if (at_meth) > + { > + /* try to build a = p.await_transform (e). */ > + tree at_fn = NULL_TREE; > + vec *args = make_tree_vector_single (expr); > + a = build_new_method_call (get_coroutine_promise_proxy ( > + current_function_decl), > + at_meth, &args, NULL_TREE, LOOKUP_NORMAL, > + &at_fn, tf_warning_or_error); > + > + /* Probably it's not an error to fail here, although possibly a bit odd > + to find await_transform but not a valid one? */ Is this comment still accurate? I don't think this is any kind of SFINAE-like context, and we will have emitted an error. > + /* Belt and braces, we should never get here, the expression should be > + required in the parser. */ > + if (expr == NULL_TREE) Then assert! > +/* placeholder; in case we really need something more than the contextual > + checks. */ Is this still needed? > +static tree > +check_co_return_expr (tree retval, bool *no_warning) > +/* Check that it's valid to have a co_return keyword here. > + If it is, then check and build the p.return_{void(),value(expr)}. > + These are built against the promise proxy, but saved for expand time. */ > + > +tree > +finish_co_return_stmt (location_t kw, tree expr) > +{ > + if (expr == error_mark_node) > + return error_mark_node; > + > + if (!coro_common_keyword_context_valid_p (current_function_decl, kw, > + "co_return")) > + return error_mark_node; > + > + /* The current function has now become a coroutine, if it wasn't > + already. */ > + DECL_COROUTINE_P (current_function_decl) = 1; > + > + if (processing_template_decl) > + { > + current_function_returns_value = 1; > + > + if (check_for_bare_parameter_packs (expr)) > + return error_mark_node; > + > + tree functype = TREE_TYPE (TREE_TYPE (current_function_decl)); > + /* If we don't know the promise type, we can't proceed, return the > + expression as it is. */ > + if (dependent_type_p (functype) || type_dependent_expression_p (expr)) > + { > + expr > + = build2_loc (kw, CO_RETRN_EXPR, void_type_node, expr, NULL_TREE); > + expr = maybe_cleanup_point_expr_void (expr); > + expr = add_stmt (expr); > + return expr; > + } > + } finish_co_{await,yield,return} all look very similar, or at least start that way. Is there an underlying helper fn waiting? > + /* If the promise object doesn't have the correct return call then > + there's a mis-match between the co_return and this. */ > + tree co_ret_call = NULL_TREE; > + if (expr == NULL_TREE || VOID_TYPE_P (TREE_TYPE (expr))) > + { > + tree crv_meth > + = lookup_promise_member (current_function_decl, "return_void", kw, > + true /*musthave*/); idiom is /*musthave=*/true > + > + expr = build2_loc (kw, CO_RETRN_EXPR, void_type_node, expr, co_ret_call); oh, please name it CO_RETURN_EXPR > +/* ================= Morph and Expand. ================= I'll review more later ... nathan -- Nathan Sidwell