Re: [C++ coroutines 3/6] Front end parsing and transforms.

[Nathan Sidwell <nathan@acm.org>]

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, "%<co_await%> 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<tree, va_gc> *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 <expr> 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