Re: [c++-concepts] code review

[Jason Merrill <jason at redhat dot com>]

> 1. tree_template_info still contains constraint_info. That will change
> in a subsequent patch. I'm kind of waiting for specializations to get
> TEMPLATE_DECLs.

Makes sense.

> 2. I left the <cstdlib> include in system.h, because removing it will
> result in errors due to an inclusion of <algorithm>. It's probable
> that both can be removed, but I didn't test it with this patch.

You mean you don't need <algorithm> anymore in logic.cc?  I think we 
want the <cstdlib> include in general if we're going to support people 
using the C++ standard library.

> +  if (DECL_USE_TEMPLATE (fn))
> +    {
> +      tree cons = DECL_TEMPLATE_CONSTRAINT (fn);
> +      if (!check_constraints (cons))
> +        {
> +          // TODO: Fail for the right reason.
> +          reason = template_unification_error_rejection ();

Can't you do

  template_constraint_failure (DECL_TI_TEMPLATE (fn), DECL_TI_ARGS (fn))

?

>    if (fn == error_mark_node)
>      {
> +      if (!check_template_constraints (tmpl, targs))
> +          reason = template_constraint_failure (tmpl, targs);
> +      else if (errorcount+sorrycount == errs)
>        /* Don't repeat unification later if it already resulted in errors.  */

A constraint failure is a form of unification failure, like SFINAE; 
let's handle it in that context rather than separately here, and reserve 
rr_constraint_failure for the case of a non-template member function.

> +// Returns the template declaration associated with the candidate
> +// function. For actual templates, this is directly associated
> +// with the candidate. For temploids, we return the template
> +// associated with the specialization.
> +static inline tree
> +get_actual_template (struct z_candidate *cand)

The uses of "actual template" in the comment and function name seem to 
mean different things.  Maybe call the function template_decl_for_candidate?

> +  // Non-temploids cannot be constrained.

Can friend temploids be constrained?

> +  if (!DECL_TEMPLOID_INSTANTIATION (new_fn) &&
> +      !DECL_TEMPLOID_INSTANTIATION (old_fn))

The && goes at the beginning of the second line.

> +// reduced terms in the constraints language. Returns NULL_TREE if either A or
>  // B are NULL_TREE.
>  tree
>  conjoin_requirements (tree a, tree b)
>  {
> -  if (a && b)
> -    return build_min (TRUTH_ANDIF_EXPR, boolean_type_node, a, b);
> +  if (a)
> +    return b ? join_requirements (TRUTH_ANDIF_EXPR, a, b) : a;
> +  else if (b)
> +    return b;
>    else
>      return NULL_TREE;

Seems like the comment is no longer accurate; the function now only 
returns NULL_TREE if both a and b are NULL_TREE.

> +// Returns true if the function decl F is a constraint predicate. It must be a
> +// constexpr, nullary function with a boolean result type.
> +static tree
> +get_constraint_check (tree call)

The comment needs updating.  And the function isn't used anywhere; it 
seems redundant with resolve_constraint_check.

> +// The raison d'entre of this class is to prevent traits

"d'etre"

> +// and other template nodes from generating new expressions
> +// during instantiation.

I'm not clear on the issue.  Perhaps leaving processing_template_decl 
alone and using fold_non_dependent_expr would accomplish what you want?

> +static inline bool
> +check_type_constraints (tree t, tree args)
> +{
> +  return check_constraints (CLASSTYPE_TEMPLATE_CONSTRAINT (t), args);
> +}
> +
> +static inline bool
> +check_decl_constraints (tree t, tree args)
> +{
> +  if (TREE_CODE (t) == TEMPLATE_DECL)
> +    return check_decl_constraints (DECL_TEMPLATE_RESULT (t), args);
> +  else
> +    return check_constraints (DECL_TEMPLATE_CONSTRAINT (t), args);
> +}
> +
> +// Check the template's constraints for the specified arguments,
> +// returning true if the constraints are satisfied and false
> +// otherwise.
> +bool
> +check_template_constraints (tree t, tree args)
> +{
> +  return check_constraints (get_constraints (t), args);
> +}

Seems like the last function would also work for types and non-template 
decls.

> +  error_at (loc, "constraints not satisfied");

I assume you're planning to make this more verbose?  It could use a TODO 
to that effect.

> +// and template isntantiation. Evaluation warnings are also inhibited.

"instantiation"

> +  else if (are_constrained_overloads (newdecl, olddecl))
> +    {
> +      // If newdecl and olddecl are function templates with the
> +      // same type but different constraints, then they cannot be
> +      // duplicate declarations. However, if the olddecl is declared
> +      // as a concept, then the program is ill-formed.
> +      if (DECL_DECLARED_CONCEPT_P (DECL_TEMPLATE_RESULT (olddecl)))
> +        {
> +          error ("cannot specialize concept %q#D", olddecl);
> +          return error_mark_node;
> +        }
> +      return NULL;
> +    }

We should check for differing constraints in decls_match, and then this 
code should be in the !types_match section of duplicate_decls.

>        error ("concept %q#D result must be convertible to bool", fn);

Why don't we require the result to actually be bool?  It seems difficult 
to decompose dependent requirements if the return type can be something 
else.

> +// Remove the current term form the list, repositioning to the term

"from"

> +//  \Gamma, P |- Delta    \Gamma, Q |- \Delta

Are the \ for TeX markup or something?  You're missing one on the left 
Delta here.

BTW, thanks: I find the rewritten logic code more readable.

> +subsumes_constraints (tree left, tree right)
> +{
> +  gcc_assert (check_constraint_info (left));
> +  gcc_assert (check_constraint_info (right));
> +
> +  // Check that c is subsumed by each subproblem in h.
> +  // If it is not, then h does not subsume c.
> +  tree as = CI_ASSUMPTIONS (left);
> +  tree c = reduce_requirements (CI_REQUIREMENTS (right));
> +  for (int i = 0; i < TREE_VEC_LENGTH (as); ++i)
> +    if (!subsumes_prop (TREE_VEC_ELT (as, i), c))
> +      return false;
> +  return true;

"h" doesn't appear anywhere in the function; let's change the comment to 
refer to "left" and "right" instead.

> +              cp_lexer_next_token_is_keyword (parser->lexer, RID_REQUIRES))
>            {
>              tree reqs = release (current_template_reqs);
> -            if (tree r = cp_parser_requires_clause_opt (parser))
> +            if (tree r = cp_parser_requires_clause (parser))

I was thinking to change the name of cp_requires_clause to 
cp_parser_requires_clause_opt and change the assert to a test for the 
keyword and return NULL_TREE if it isn't found.

> -              if (flag_concepts
> -                  && function_declarator_p (declarator)
> +              if (flag_concepts &&
> +                  function_declarator_p (declarator) &&

The && was in the right place before this change.

> +  if (!potential_rvalue_constant_expression (expr))
> +    {
> +      require_potential_rvalue_constant_expression (expr);

This can just be if (!require_pot....

> +          // FIXME: we should be checking the constraints, not just
> +          // instantiating them.

Let's check them in determine_specialization, along with the SFINAE 
check after the comment
         /* Make sure that the deduced arguments actually work.  */
Can explicit specializations have constraints, to indicate which 
template they are specializing?

> +      // FIXME: This points to the wrong line.
> +      error_at (input_location, "redeclaration %q+D with different constraints", tmpl);

%q+D is telling the diagnostic code to use DECL_SOURCE_LOCATION (tmpl) 
as the location.  I think you want

error_at (input_location, "redeclaration of %q#D with different "
          "constraints", tmpl);
inform (DECL_SOURCE_LOCATION (tmpl),
        "original declaration appeared here");

> +  tree argcons = TREE_CODE (argtype) == TEMPLATE_TEMPLATE_PARM
> +    ? DECL_TEMPLATE_CONSTRAINT (DECL_TEMPLATE_RESULT (parm))
> +    : TYPE_TEMPLATE_CONSTRAINT (argtype);

Seems like get_constraints would be useful here.

> +                        error ("constraint mismatch at argument %d in "
> +                               "template parameter list for %qD",
> +                               i + 1, in_decl);

Let's also print the parameter and argument.

> +// Argument deduction ios applied to all template arguments, and

Passing 'true' for require_all_args means no deduction will be done; 
rather, all arguments must either be specified or have default arguments.

> +      // Check class template requirements. Note that constraints will
> +      // already have been checked when trying to find a most specialized
> +      // class among multiple specializations.

I don't see where that is; more_specialized_class doesn't seem to 
compare constraints yet.

> +        tree cons = DECL_CLASS_TEMPLATE_P (t)
> +          ? TYPE_TEMPLATE_CONSTRAINT (type)
> +          : DECL_TEMPLATE_CONSTRAINT (decl);

get_constraints would be useful here as well.

> +            // FIXME: Do we need to attach constraints?
> +            // TODO: Is this actually necessary?

I don't think you need to fill in TI_CONSTRAINT for anything but the 
main template patterns, here or other places.

> +substitute_requirements (tree reqs, tree args)

This seems redundant with instantiate_requirements.

> +  // Build template info and associate it with

Incomplete sentence.

> +  // TODO: Do we need to propagate constaints into bound template
> +  // template parameters? Probably not for interface checking.

"constraints"

But I agree probably not, for the same reason we don't need constraints 
on instantiated FIELD_DECLs.

Jason