Concepts code review

Jason Merrill jason@redhat.com
Tue Nov 11 17:09:00 GMT 2014


I'm reading over the concepts code now with a view toward merging in the 
next few weeks.  We don't need to address all of my comments before 
then, but it would be good to get started.

> +  // 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.

> +  // 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.

>   // Concept declarations must have a corresponding definition.
>   //
>   // TODO: This should be part of the up-front checking for
>   // a concept declaration.
>   if (!DECL_SAVED_TREE (decl))
>     {
>       error_at (DECL_SOURCE_LOCATION (decl),
>                 "concept %q#D has no definition", decl);
>       return NULL;
>     }

Check funcdef_flag in grokfndecl?

> +resolve_constraint_check (tree call)
> +{
> +  gcc_assert (TREE_CODE (call) == CALL_EXPR);

Maybe also assert that the call has no function arguments?

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

Can you reduce the code duplication between deduce_constrained_parameter 
and deduce_concept_introduction?

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

> +  // 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;

Remove the commented declaration and adjust the comment as appropriate? 
  For that matter, is check_logical actually doing anything currently? 
It seems to be called with an uninstantiated (dependent) expression, 
since we're normalizing before substitution.

> +  // 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.

> +      // FIXME: input_location is probably wrong, but there's not necessarly
> +      // an expr location with the tree.

You can use EXPR_LOC_OR_LOC (t, input_location).

> }
>
> tree
> build_requires_expr (tree parms, tree reqs)

Needs a comment.

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

> eval_requires_expr (tree reqs)
> {
>   for (tree t = reqs ; t; t = TREE_CHAIN (t))
>     {
>       tree r = TREE_VALUE (t);
>       r = fold_non_dependent_expr (r);

If we're only calling this in non-template context, this call will 
always be a no-op.

It might be good to add a warning about non-dependent requirements on a 
template (somewhere, not here).

> // FIXME: Semantics need to be aligned with the new version of the
> // specification (i.e., we must be able to invent a function and
> // perform argument deduction against it).

'auto' deduction uses the rules for template deduction for a function 
call, so you can use do_auto_deduction.

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

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

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

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

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

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

> // Diagnose constraint failures in a variable concept.
> void
> diagnose_var (location_t loc, tree t, tree args)

The name and comment seem misleading since T is actually a TEMPLATE_ID_EXPR.

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

> +//    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.

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

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

> +// 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.

>   // Convert the TREE_LIST into a TREE_VEC, similar to what is done in
>   // end_template_parm_list.

Let's use a vec<tree> (from make_tree_vector) rather than TREE_LIST for 
the temporary.

> +cp_get_id_declarator (cp_declarator *declarator)

There are a lot of new functions beginning with cp_ here, but the 
pattern in parser.c is to start with cp_parser; let's add the "parser" 
to functions that are doing parsing, and drop the "cp" from functions 
that aren't.  For this function let's use "get_declarator_id".

> +cp_get_identifier (cp_declarator *declarator)

And here, "get_unqualified_id".

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

>   // FIXME: This should probably be copied, and we may need to adjust
>   // the template parameter depths.

Yep.  Probably easier to build up new parameters based on the old ones 
rather than copy and adjust, but perhaps not.

> +cp_parser_default_type_template_argument (cp_parser *parser)
> +cp_parser_default_template_template_argument (cp_parser *parser)

It looks like these were copied out of cp_parser_type_parameter, but the 
code is also still in that function; it should be replaced with calls to 
these.

> +  // TODO: Actually bind the constraint to the auto.

Maybe use build_constrained_parameter here, too, and only call make_auto 
when checking the requirement?

> +  // Enter a scope of kind K belonging to the decl D.
> +  cp_parser_requires_expr_scope ()
> +  {
> +    begin_scope (sk_block, NULL_TREE);

The comment doesn't seem to match the code.

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

> +  // TODO: Earlier versions of the concepts lite spec did not allow
> +  // requires expressions outside of template declarations. That
> +  // restriction was relaxed in Chicago, but it has not been implemented.
> +  if (!processing_template_decl)
> +    {
> +      error_at (loc, "a requires expression cannot appear outside a template");
> +      cp_parser_skip_to_end_of_statement (parser);
> +      return error_mark_node;
> +    }
> +
> +
> +  // TODO: Check that requires expressions are only written inside of
> +  // template declarations. They don't need to be concepts, just templates.

Isn't the second TODO covered by the code immediately before it?

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

> +      // 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.

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

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

> +      // Save the current template requirements.
> +      saved_template_reqs = release (current_template_reqs);

It seems like a lot of places with saved_template_reqs variables could 
be converted to use cp_manage_requirements.

> +// 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).

> +  /* 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.  :)

> +  // 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.

More later.

OK to apply these changes?

Jason
-------------- next part --------------
A non-text attachment was scrubbed...
Name: concepts-tweaks.patch
Type: text/x-patch
Size: 6268 bytes
Desc: not available
URL: <http://gcc.gnu.org/pipermail/gcc-patches/attachments/20141111/fd3c578a/attachment.bin>


More information about the Gcc-patches mailing list