[c++-concepts] code review

[Jason Merrill <jason@redhat.com>]

It looks like things are coming together pretty well.  What's your 
feeling about readiness to merge into the trunk?  Is the branch down to 
no regressions?

See you on Monday!

> @@ -4146,21 +4146,21 @@ build_new_function_call (tree fn, vec<tree, va_gc> **args, bool koenig_p,
>        if (TREE_CODE (fn) == TEMPLATE_ID_EXPR)
>          {
>            /* If overload resolution selects a specialization of a
> +             function concept for non-dependent template arguments,
> +             the expression is true if the constraints are satisfied
> +             and false otherwise.
>
>               NOTE: This is an extension of Concepts Lite TS that
>               allows constraints to be used in expressions. */
> +          if (flag_concepts && !processing_template_decl)
>              {
>                tree tmpl = DECL_TI_TEMPLATE (cand->fn);
> +              tree targs = DECL_TI_ARGS (cand->fn);
>                tree decl = DECL_TEMPLATE_RESULT (tmpl);
> +              if (DECL_DECLARED_CONCEPT_P (decl)
> +                  && !uses_template_parms (targs)) {
> +                return evaluate_function_concept (decl, targs);

If processing_template_decl is false, uses_template_parms should always 
be false as well.

> +function_concept_check_p (tree t)

> +  tree fn = CALL_EXPR_FN (t);
> +  if (TREE_CODE (fn) == TEMPLATE_ID_EXPR
> +      && TREE_CODE (TREE_OPERAND (fn, 0)) == OVERLOAD)
> +    {
> +      tree f1 = OVL_FUNCTION (TREE_OPERAND (fn, 0));

I think you want get_first_fn here.

>    if (TEMPLATE_PARM_CONSTRAINTS (current_template_parms))
> -    TYPE_CANONICAL (type) = type;
> +    SET_TYPE_STRUCTURAL_EQUALITY (type);

This seems like papering over an underlying issue.  What was the 
testcase that motivated this change?

> @@ -11854,7 +11854,7 @@ tsubst_decl (tree t, tree args, tsubst_flags_t complain)
> -           if (!spec)
> +           if (!spec && DECL_LANG_SPECIFIC (t))
> -       if (!local_p)
> +       if (!local_p && DECL_LANG_SPECIFIC (r))

What motivated these changes?  From the testcase, it seems that you're 
getting here with the decl for "using TD = int", which shouldn't happen.

> @@ -1159,7 +1159,6 @@ check_noexcept_r (tree *tp, int * /*walk_subtrees*/, void * /*data*/)
> -      tree type = TREE_TYPE (TREE_TYPE (fn));
> -      if (!TYPE_NOTHROW_P (type))
> +      if (!TYPE_NOTHROW_P (TREE_TYPE (fn)))

The old code was incorrectly assuming that CALL_EXPR_FN is always a 
function pointer, but your new code seems to be incorrectly assuming 
that it's always a function or an expression taking the address of a 
function; I think this will break on a call to a function pointer variable.

> @@ -3481,13 +3481,27 @@ cxx_eval_constant_expression (const constexpr_ctx *ctx, tree t,
>      case REQUIRES_EXPR:
> +      if (!processing_template_decl)
> +        return evaluate_constraint_expression (t, NULL_TREE);
> +      else
> +        *non_constant_p = true;
> +        return t;

We shouldn't get here with a dependent REQUIRES_EXPR (or any dependent 
expression), so we shouldn't ever hit the else clause.

> @@ -18063,18 +18063,41 @@ cp_parser_declarator (cp_parser* parser,
> +  /* Function declarations may be followed by a trailing
> +     requires-clause. Declarators for function declartions
> +     are function declarators wrapping an id-declarator.
> +     If the inner declarator is anything else, it does not
> +     declare a function. These may also be reference or
> +     pointer declarators enclosing such a function declarator.
> +     In the declaration :
> +
> +        int *f(args)
> +
> +     the declarator is *f(args).
> +
> +     Abstract declarators cannot have a requires-clauses
> +     because they do not declare functions. Here:
>
>          void f() -> int& requires false
>
> +     The trailing return type contains an abstract declarator,
> +     and the requires-clause applies to the function
> +     declaration and not the abstract declarator.  */
> +  if (flag_concepts && dcl_kind != CP_PARSER_DECLARATOR_ABSTRACT)
>      {
> +      /* We could have things like *f(args) or &f(args).
> +         Look inside references and pointers.  */
> +      cp_declarator* p = declarator;
> +      if (p->kind == cdk_reference || p->kind == cdk_pointer)
> +        p = p->declarator;
> +
> +      /* Pointers or references with no name, or functions
> +         with no name cannot have constraints.  */
> +      if (!p || !p->declarator)
> +        return declarator;
> +
> +      /* Look for f(args) but not (*f)(args).  */
> +      if (p && p->kind == cdk_function && p->declarator->kind == cdk_id)

I think you can use function_declarator_p here.

> +static inline bool
> +pending_expansion_p (tree t)
> +{
> +  return (TREE_CODE (t) == PARM_DECL && CONSTRAINT_VAR_P (t)
> +          && PACK_EXPANSION_P (TREE_TYPE (t)));
> +}

What's the difference between this and function_parameter_pack_p?

> +  /* A sentinel class that ensures that deferred access checks
> +     are popped before a function returns.  */
> +  struct deferring_access_check_sentinel
> +  {
> +    deferring_access_check_sentinel ()
> +    {
> +      push_deferring_access_checks (dk_deferred);
> +    }
> +    ~ deferring_access_check_sentinel ()
> +    {
> +      pop_deferring_access_checks ();
> +    }
> +  }

Let's put this with the other RAII sentinels in cp-tree.h.

> +                       Lifting of concept definitions

Could we have a bit more description of what "lifting" means here?

> +/*---------------------------------------------------------------------------
> +                        Constraint normalization
> +---------------------------------------------------------------------------*/

You have two of these headers; I guess the first one should be 
"transformation" and could use more description.  On the second, I would 
retain the old comment

> -// Normalize a template requirement to a logical formula written in terms of
> -// atomic propositions, returing the new expression.  If the expression cannot
> -// be normalized, a NULL_TREE is returned.

> +check_implicit_conversion_constraint (tree t, tree args,
> +                                      tsubst_flags_t complain, tree in_decl)
> +{
> +  tree expr = ICONV_CONSTR_EXPR (t);
> +
> +  /* Don't tsubst as if we're processing a template. If we try
> +     to we can end up generating template-like expressions
> +     (e.g., modop-exprs) that aren't properly typed. */
> +  int saved_template_decl = processing_template_decl;
> +  processing_template_decl = 0;

Why are we checking constraints when processing_template_decl is true?

> +   Note that this is the only place that we instantiate the
> +   constraints. */
> +bool
> +check_constraints (tree ci, tree args)

Except for tsubst_constraint?

> +  ++processing_template_decl;
> +  tree constr = transform_expression (lift_function_definition (fn, args));
> +  --processing_template_decl;

Why do you need to set processing_template_decl here and in other calls 
to transform_expression?  I don't notice anything that would be helped, 
especially now that you're using separate tree codes for constraints, 
though there is this in check_logical_expr:

> +  /* Resolve the logical operator. Note that template processing is
> +     disabled so we get the actual call or target expression back.
> +     not_processing_template_sentinel sentinel.

I guess that isn't needed anymore?

> +   FIXME: This is defined in pt.c because it's garbage collection
> +   code is not being generated for constraint.cc. */
> +static GTY (()) hash_table<constr_hasher> *decl_constraints;

I think you need to add constraint.cc to the gtfiles variable in 
cp/config-lang.in.  It also looks like the GTFILES_H rule in 
gcc/Makefile.in assumes that all sources have the .c extension, so that 
may need adjustment.

> @@ -2454,6 +2454,8 @@ finish_call_expr (tree fn, vec<tree, va_gc> **args, bool d
>               next = OVL_CHAIN (fn);
> +              if (flag_concepts)
> +                remove_constraints (OVL_FUNCTION (fn));

I don't think we want to remove constraints here; this code is just 
discarding the OVERLOAD node, not the FUNCTION_DECL.

Jason