Re: Concepts code review

[Andrew Sutton <andrew dot n dot sutton at gmail dot com>]

>> +  // The constraint info maintains information about constraints
>> +  // associated with the declaration.
>> +  tree constraint_info;
>
>
> We talked back at the end of June about moving this into a separate
> hashtable; I'm still reluctant to add another pointer to most declarations
> when only a minority will have constraints.  As I was saying in the earlier
> thread, I think the problem you were hitting should be resolved by looking
> through clones with DECL_ORIGIN.  This needs to be fixed before merge, since
> it significantly affects non-concepts compiles.


Agreed. I'll probably start looking at this on Friday morning.


>> +  // Zeroth, a constrained function is not viable if its constraints are
>> not
>> +  // satisfied.
>
> As I suggested in the document review, I think we want to check this after
> the number of parameters, to avoid unnecessary implicit template
> instantiation in evaluating the constraints.  Patch attached.


Great. I'll apply that after I merge with trunk (which is going on right now).



>> +resolve_constraint_check (tree call)
>> +{
>> +  gcc_assert (TREE_CODE (call) == CALL_EXPR);
>
>
> Maybe also assert that the call has no function arguments?


I'll have to look, but we might get regular calls to constexpr
functions through this code path, which are then filtered out during
processing.

It might be better to not take this path if there are function
arguments since that couldn't possibly be a constraint check.


>> deduce_concept_introduction (tree expr)
>> {
>>   if (TREE_CODE (expr) == TEMPLATE_ID_EXPR)
>>     {
>>       // Get the parameters from the template expression.
>>       tree decl = TREE_OPERAND (expr, 0);
>>       tree args = TREE_OPERAND (expr, 1);
>>       tree var = DECL_TEMPLATE_RESULT (decl);
>>       tree parms = TREE_VALUE (DECL_TEMPLATE_PARMS (decl));
>>
>>       parms = coerce_template_parms (parms, args, var);
>
>
> Do you still need this coerce_template_parms now that I've added a call to
> lookup_template_variable?  Well, once my change is merged onto the branch or
> the branch onto trunk.


Maybe? I'm not sure what the call to lookup_template_variable is going
to do :) I think we still need to instantiate default arguments in
order to perform the match.


> Can you reduce the code duplication between deduce_constrained_parameter and
> deduce_concept_introduction?


Yes.

>>   // Sometimes a function call results in the creation of clean up
>>   // points. Allow these to be preserved in the body of the
>>   // constraint, as we might actually need them for some constexpr
>>   // evaluations.
>
>
> What need are you thinking of?  CLEANUP_POINT_EXPR is ignored in constexpr
> evaluation.
>
> Also, this function seems like reinventing massage_constexpr_body, can we
> share code?


I didn't know if the forthcoming generalized constexpr evaluator would
act on those or not. We'll have a look at the massage_constexpr_body
function. I wonder if we can apply that to both function and constexpr
variables.


>> +  // Normalize the body of the function into the constriants language.
>> +  tree body = normalize_constraints (DECL_SAVED_TREE (fn));
>> +  if (!body)
>> +    return error_mark_node;
> ...
>>
>> +  // Reduce the initializer of the variable into the constriants
>> language.
>> +  tree body = normalize_constraints (DECL_INITIAL (decl));
>
>
> If we're normalizing before substitution, why wait until the point of use to
> do it?  At least we could cache the result of normalization.


We should be normalizing and caching as soon as we can create a
complete declaration (for some declaration of complete). For functions
and function templates, for example, that's at the top of grokfndecl.

Although, as I think about it, I seem to remember having to
re-normalize a constraint in certain circumstances, but I can't
remember what they are. I'll take a look at this.


>>   // Modify the declared parameters by removing their context (so they
>>   // don't refer to the enclosing scope), and marking them constant (so
>>   // we can actually check constexpr properties).
>
>
> We don't check constexpr using these parms anymore, but rather the
> substituted arguments, so we don't need to mark them constant, right? Is the
> other (context) change still relevant?


That will come out.


>> +  // TODO: Actually check that the expression can be constexpr
>> +  // evaluatd.
>> +  //
>> +  // return truth_node (potential_constant_expression (expr));
>> +  sorry ("constexpr requirement");
>
>
> Pass it to maybe_constant_value and check whether the result is
> TREE_CONSTANT?  Or do we want to remove the constexpr requirement code now
> that it's out of the proposal?


This should come out. I think we'll get a non-language way of checking
this in the not-so-distant future.


>>   DECL_INITIAL (decl) = proto;  // Describing parameter
>>   DECL_SIZE_UNIT (decl) = fn;   // Constraining function declaration
>>   DECL_SIZE (decl) = args;      // Extra template arguments.
>
>
> I'm nervous about misusing these fields this way.  Why is a constrained
> parameter represented as a TYPE_DECL?


I think it was because the callers of cp_parser_nonclass_name (?)
expect to get back some kind of type or type-decl. This was the most
direct way to make constrained-type-specifiers and
constrained-parameters work (I did this at the meeting in Issaquah as
a proof of concept).

I don't especially like it either. Any recommendations?


>> +// In an unevaluated context, the substitution of parm decls are not
>> +// properly chained during substitution. Do that here.
>
>
> Do we actually need them chained, since we're only putting them in
> local_specializations?


I ran into some problems early when we weren't chaining these. It's
possible that I did this before adding them to local_specializations.
I don't know.


>> +  while (t)
>> +    {
>> +      tree e = tsubst_expr (TREE_VALUE (t), args, tf_none, in_decl,
>> false);
>> +      if (e == error_mark_node)
>> +        e = boolean_false_node;
>> +      r = tree_cons (NULL_TREE, e, r);
>> +      t = TREE_CHAIN (t);
>> +    }
>
>
> Maybe return early once we see an error rather than continuing to check the
> other requirements?


Good idea.

>> +// Used in various contexts to control substitution. In particular, when
>> +// non-zero, the substitution of NULL arguments into a type will still
>> +// process the type as if passing non-NULL arguments, allowing type
>> +// expressions to be fully elaborated during substitution.
>> +int processing_constraint;
>
>
> During substitution we should have non-NULL arguments.  What sort of
> situation requires this?


fold_non_dependent_expr_sfinae explicitly calls tsubst_copy_and_build
with NULL arguments.

I think the processing_constraint comes in from the normalization
rules, where substitute dependently. Certain computations (I don't
remember which) weren't happening in types because there's an early
check for NULL arguments that just returns the input tree.

I'll look to make sure that this is actually necessary.


>> +  // Instantiate and evaluate the requirements.
>> +  req = tsubst_constraint_expr (req, args, false);
>> +  if (req == error_mark_node)
>> +    return false;
>> +
>> +  // Reduce any remaining TRAIT_EXPR nodes before evaluating.
>> +  req = fold_non_dependent_expr (req);
>
>
> Again, this should have no effect outside of a template.  Is it possible to
> get here in template context?


Probably not any more. We should only be entering the check when it's
known that the template arguments are not dependent.


>> +  // TODO: This isn't currently possible, but it will almost certainly
>> +  // change with variable templates.
>> +  tree d1 = DECL_TEMPLATE_RESULT (olddecl);
>> +  tree d2 = DECL_TEMPLATE_RESULT (newdecl);
>> +  if (TREE_CODE (d1) != TREE_CODE (d2))
>> +    return false;
>
>
> Does this need anything other than removing the comment?


Nope.


>> +//    template<typename T>
>> +//    template<C U>
>> +//    void S<T>::f() { }  // #2
>
>> +// When grokking #2, the constraints introduced by C are not
>> +// in the current_template_reqs; they are attached to the innermost
>> +// parameter list. The adjustment makes them part of the current
>> +// template requirements.
>
>
> Why is this necessary for member function templates and not member class
> templates?  What is removing the innermost requirements from
> current_template_reqs?
>
> It seems like TEMPLATE_PARMS_CONSTRAINTS includes requirements from outer
> template parameter-lists.  Given that, what was the motivation for:
>
>> +       (adjust_fn_constraints): Rewrite so that class template
>> constraints
>> +       are not imposed on member function declarations.
>
>
> ?  Why are class template constraints imposed on member templates but not
> non-template member functions?  I would think we would want to be consistent
> by either always or never imposing them on members or never.
>
> This could be clearer in the proposal, too: how requirements bind to
> template parameter lists vs scopes vs declarations.


I think this has always been a little bit unclear to me and always a
little bit broken. I'll take another pass on this.


>> +          // Do not allow the declaration of constrained friend template
>> +          // specializations. They cannot be instantiated since they
>> +          // must match a fully instantiated function, and non-dependent
>> +          // functions cannot be constrained.
>
>
> I guess this is no longer accurate now that we're allowing constraints on
> non-template functions.  That applies to specializations as well, right?


Right.

>
>>                 if (concept_p)
>>                   // TODO: This needs to be revisited once variable
>>                   // templates are supported
>>                     error ("static data member %qE declared %<concept%>",
>>                            unqualified_id);
>
>
> Just remove the comment?


Right.


>> +// Bring the parameters of a function declaration back into
>> +// scope without entering the function body. The declarator
>> +// must be a function declarator. The caller is responsible
>> +// for calling finish_scope.
>> +void
>> +push_function_parms (cp_declarator *declarator)
>
>
> I think if the caller is calling finish_scope, I'd prefer for the
> begin_scope call to be there as well.


We moved the requires clause into a function declarator last week, so
this will probably change.


>> +  tree placeholder = build_nt (PLACEHOLDER_EXPR);
>
>
> Using PLACEHOLDER_EXPR is likely to be more complicated now that I'm using
> it for references to the current object in NSDMI.  Any reason not to use
> INTRODUCED_PARM_DECL here as well?


Not at all. The use of PLACEHOLDER predates INTRODUCED_PARM_DECL by
about a year :)


>> +  // TODO: Actually bind the constraint to the auto.
>
>
> Maybe use build_constrained_parameter here, too, and only call make_auto
> when checking the requirement?


Maybe. I forget the specifics of how auto processing actually works,
but that sounds reasonable.


>> +    for (tree t = current_binding_level->names; t; t = DECL_CHAIN (t))
>> +      pop_binding (DECL_NAME (t), t);
>> +    leave_scope ();
>
>
> This is pop_bindings_and_leave_scope.

Good to know!


>> +  // Parse the optional parameter list. Any local parameter declarations
>> +  // are added to a new scope and are visible within the nested
>> +  // requirement list.
>
>
> I'm surprised that the parens are necessary in the current proposal.  I
> assume that was discussed in Evolution?


For the requires-expression? No discussion in SG8 or EWG about those
parens that I recall.

We can make them optional in the implementation and discuss it in the
next telecon.


>> +      // If we see a semi-colon, consume it.
>> +      if (cp_lexer_next_token_is (parser->lexer, CPP_SEMICOLON))
>> +          cp_lexer_consume_token (parser->lexer);
>
>
> This doesn't seem to require that requirements be separated by semicolons,
> as we also noticed when reviewing the proposal.


Will fix.


>> +static tree
>> +cp_parser_constraint_spec (cp_parser *parser, tree expr)
>> +{
>> +  if (cp_lexer_next_token_is_keyword (parser->lexer, RID_CONSTEXPR))
>> +    return cp_parser_constexpr_constraint_spec (parser, expr);
>> +  if (cp_lexer_next_token_is_keyword (parser->lexer, RID_NOEXCEPT))
>> +    return cp_parser_noexcept_constraint_spec (parser, expr);
>> +  return NULL_TREE;
>> +}
>
>
> Instead of building up separate CONSTEXPR_EXPR and NOEXCEPT_EXPR nodes,
> maybe make them flags on the VALIDEXPR_EXPR?


CONSTEXPR_EXPR will go away entirely. We can make the noexcept a flag.


>> +  if (tree p = finish_concept_introduction (tmpl_decl,
>> introduction_list))
>> +    return p;
>> +
>> +  error_at (token->location, "no matching concept for
>> introduction-list");
>
>
> Seems like this could use a diagnostic about tmpl_decl not being a concept.
> I suppose other places could as well, so I guess we want a common function
> to check whether the lookup result is a concept and complain otherwise.


In the case of constrained-type-specifiers, we tend to just return
whatever we find and let the usual processing rules diagnose the
constraint (e.g., using a non-type name where a type is expected).

There are cases where trying to be proactive about diagnosing those
lookups broke a lot of existing parses, specifically the
postfix-expression for constructor calls. For example, in C(args), if
T finds

template<typename T>
  concept bool C() { return true; }
template<typename T>
  int C(int n) { return n; }

We to select C(int).

The check for introductions might be a special case.


>> +// TODO: This is a bit of a hack. When we finish the template parameter
>> +// the constraint is just a call expression, but we don't have the full
>> +// context that we used to build that call expression. Since we're going
>> +// to be comparing declarations, it would helpful to have that. This
>> +// means we'll have to make the TREE_TYPE of the parameter node a pair
>> +// containing the context (the TYPE_DECL) and the constraint.
>
>
> This doesn't seem to be describing anything in the function that follows
> (get_concept_from_constraint).


An old comment that wasn't removed, I think.

>
>> +  /* True if parsing a template parameter list. The interpretation of a
>> +     constrained-type-specifiers differs inside a template parameter
>> +     list than outside. */
>> +  bool in_result_type_constraint_p;
>
>
> The comment seems inaccurate.  :)

Yes, it does. And I don't remember where that code came from :)


>> +  // FIXME: This could be improved. Perhaps the type of the requires
>> +  // expression depends on the satisfaction of its constraints. That
>> +  // is, its type is bool only if its substitution into its normalized
>> +  // constraints succeeds.
>
>
> The requires-expression is not type-dependent, but it can be
> instantiation-dependent and value-dependent.


Yeah, that's totally wrong. Its certainly value-dependent.

> OK to apply these changes?

Yup. Looks good to me.

Andrew