Re: [c++-concepts] code review

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

Today is the first day I've had to look at these comments.


>>    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?


It almost certainly is, but I haven't been able to find or write a
minimal test case that reproduces the reason for failure. Basically,
we end up with multiple specializations having the same type but
different constraints (since constraints are attached to the
declaration and not the type itself).

I think that I'm running into the same problems with auto a
placeholder in recent commits.


>> @@ -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.


That's the pretty much it... I suppose we could guard against
substituting into these kinds of declarations from within
tsubst_type_requirement and satisfy_type_constraint. To me it seems
like tsubst should work, but just return the same thing.


>> @@ -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.


I will experiment.


>> @@ -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.


IIRC we get here because of build_x_binary_op. It tries to build
non-dependent operands when the operands are not type-dependent.
requires-expressions have type bool, so they get run through the
constexpr evaluator even when processing_template_decl is true.

I've made requires-expressions instantiation dependent, but that
doesn't help in this case.


>> +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?


Not a lot, except that replacing pending_expansion_p in one of the two
places that it's used causes ICEs :)  This function can almost
certainly be removed.


>> +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?


IIRC I allow constraints to be evaluated in any context because it
lets us catch these kinds of errors:

template<typename T>
void f()
{
  vector<int&> v; // error: constraints not satisfied
}


>> +  ++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?


I've had problems in the past where substitution tries a little too
eagerly to fold expressions into constants --- especially type traits.
Those need to be preserved in the text for ordering.

Although I think this really only matters when you're instantiating a
class template whose members are constraints.


Andrew