Concepts code review

Concepts code review

[Jason Merrill]

[-- Attachment #1: Type: text/plain, Size: 15165 bytes --]

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

[-- Attachment #2: concepts-tweaks.patch --]
[-- Type: text/x-patch, Size: 6268 bytes --]

commit 22b8eb0f5ec72267a64b47d425522ebfc5aa174c
Author: Jason Merrill <jason@redhat.com>
Date:   Mon Nov 3 15:19:06 2014 -0600

    	* gcc/cp/class.c (get_member_fn_template)
    	(are_constrained_member_overloads): Remove.
    	(add_method): Call equivalently_constrained directly.

--- a/ChangeLog.concepts
+++ b/ChangeLog.concepts
@@ -1,3 +1,9 @@
+2014-11-11  Jason Merrill  <jason@redhat.com>
+
+	* gcc/cp/class.c (get_member_fn_template)
+	(are_constrained_member_overloads): Remove.
+	(add_method): Call equivalently_constrained directly.
+
 2014-11-03  Jason Merrill  <jason@redhat.com>
 
 	* gcc/cp/parser.c (cp_parser_nonclass_name): Fix merge error.
diff --git a/gcc/cp/class.c b/gcc/cp/class.c
index bb451cd..c519fc5 100644
--- a/gcc/cp/class.c
+++ b/gcc/cp/class.c
@@ -961,41 +961,6 @@ modify_vtable_entry (tree t,
       BV_FN (v) = fndecl;
     }
 }
-
-// Returns the template associated with the member FN or
-// NULL if the declaration is neither a template nor temploid.
-static inline tree
-get_member_fn_template (tree fn)
-{
-  if (TREE_CODE (fn) == TEMPLATE_DECL)
-    return fn;
-  if (TREE_CODE (fn) == FUNCTION_DECL && DECL_TEMPLATE_INFO (fn))
-    return DECL_TI_TEMPLATE (fn);
-  return NULL_TREE;
-}
-
-// Returns true if NEWDECL and OLDDECL are member functions with with 
-// different constraints. If NEWDECL and OLDDECL are non-template members
-// or specializations of non-template members, the overloads are 
-// differentiated by the template constraints.
-//
-// Note that the types of the functions are assumed to be equivalent.
-static inline bool
-are_constrained_member_overloads (tree newdecl, tree olddecl) 
-{
-  return !equivalently_constrained (newdecl, olddecl);
-
-  // newdecl = get_member_fn_template (newdecl);
-  // olddecl = get_member_fn_template (olddecl);
-
-  // // If neither is a template or temploid, then they cannot
-  // // be constrained declarations.
-  // if (!newdecl && !olddecl)
-  //   return false;
-  // else
-  //   return !equivalently_constrained (newdecl, olddecl);
-}
-
 \f
 /* Add method METHOD to class TYPE.  If USING_DECL is non-null, it is
    the USING_DECL naming METHOD.  Returns true if the method could be
@@ -1153,7 +1118,8 @@ add_method (tree type, tree method, tree using_decl)
       if (compparms (parms1, parms2)
 	  && (!DECL_CONV_FN_P (fn)
 	      || same_type_p (TREE_TYPE (fn_type),
-			      TREE_TYPE (method_type))))
+			      TREE_TYPE (method_type)))
+          && equivalently_constrained (fn, method))
 	{
 	  /* For function versions, their parms and types match
 	     but they are not duplicates.  Record function versions
@@ -1202,8 +1168,6 @@ add_method (tree type, tree method, tree using_decl)
 		/* Defer to the local function.  */
 		return false;
 	    }
-          else if (are_constrained_member_overloads (fn, method))
-            continue;
 	  else
 	    {
 	      error ("%q+#D cannot be overloaded", method);

commit 71f0c3b6250cd29d73a03cdcc2cb833bae25ba87
Author: Jason Merrill <jason@redhat.com>
Date:   Tue Nov 4 17:00:15 2014 -0600

    	* call.c (add_function_candidate): Move constraint check after
    	arity check.

--- a/ChangeLog.concepts
+++ b/ChangeLog.concepts
@@ -1,5 +1,8 @@
 2014-11-11  Jason Merrill  <jason@redhat.com>
 
+	* gcc/cp/call.c (add_function_candidate): Move constraint check after
+	arity check.
+
 	* gcc/cp/class.c (get_member_fn_template)
 	(are_constrained_member_overloads): Remove.
 	(add_method): Call equivalently_constrained directly.
diff --git a/gcc/cp/call.c b/gcc/cp/call.c
index fe23039..c30fd70 100644
--- a/gcc/cp/call.c
+++ b/gcc/cp/call.c
@@ -1960,21 +1960,6 @@ add_function_candidate (struct z_candidate **candidates,
   len = vec_safe_length (args) - skip + (first_arg != NULL_TREE ? 1 : 0);
   convs = alloc_conversions (len);
 
-  // Viable functions
-  //
-  // Zeroth, a constrained function is not viable if its constraints are not
-  // satisfied.
-  if (flag_concepts) {
-    if (tree ci = get_constraints (fn)) {
-      if (!check_constraints (ci))
-        {
-          reason = constraint_failure (fn);
-          viable = false;
-          goto out;
-        }
-    }
-  }
-
   /* 13.3.2 - Viable functions [over.match.viable]
      First, to be a viable function, a candidate function shall have enough
      parameters to agree in number with the arguments in the list.
@@ -1997,10 +1982,23 @@ add_function_candidate (struct z_candidate **candidates,
       viable = 0;
       reason = arity_rejection (first_arg, i + remaining, len);
     }
+
+  // Second, a constrained function is not viable if its constraints are not
+  // satisfied.
+  if (viable && flag_concepts) {
+    if (tree ci = get_constraints (fn)) {
+      if (!check_constraints (ci))
+        {
+          reason = constraint_failure (fn);
+          viable = false;
+        }
+    }
+  }
+
   /* When looking for a function from a subobject from an implicit
      copy/move constructor/operator=, don't consider anything that takes (a
      reference to) an unrelated type.  See c++/44909 and core 1092.  */
-  else if (parmlist && (flags & LOOKUP_DEFAULTED))
+  if (viable && parmlist && (flags & LOOKUP_DEFAULTED))
     {
       if (DECL_CONSTRUCTOR_P (fn))
 	i = 1;
@@ -2024,7 +2022,7 @@ add_function_candidate (struct z_candidate **candidates,
   if (! viable)
     goto out;
 
-  /* Second, for F to be a viable function, there shall exist for each
+  /* Third, for F to be a viable function, there shall exist for each
      argument an implicit conversion sequence that converts that argument
      to the corresponding parameter of F.  */
 

commit 7615fdfe089206eedb63681a3a51790638444a3a
Author: Jason Merrill <jason@redhat.com>
Date:   Tue Nov 11 11:43:04 2014 -0500

    	* g++.dg/concepts/var-concepts3.C: Fix C2.

diff --git a/gcc/testsuite/g++.dg/concepts/var-concepts3.C b/gcc/testsuite/g++.dg/concepts/var-concepts3.C
index b8f0927..010a96e 100644
--- a/gcc/testsuite/g++.dg/concepts/var-concepts3.C
+++ b/gcc/testsuite/g++.dg/concepts/var-concepts3.C
@@ -4,7 +4,7 @@ template<typename T>
   concept bool C1 = __is_class(T);
 
 template<typename T>
-  concept bool C2() { __is_class(T); }
+  concept bool C2() { return __is_class(T); }
 
 template<typename T>
   constexpr bool C3 = __is_class(T);

Re: Concepts code review

[Andrew Sutton]

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

Re: Concepts code review

[Jason Merrill]

On 11/12/2014 10:27 AM, Andrew Sutton wrote:
> Agreed. I'll probably start looking at this on Friday morning.

Note that end of stage 1 is Saturday, as I just realized today.  So the 
sooner the better.  :)

>>> deduce_concept_introduction (tree expr)

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

I meant that lookup_template_variable now calls coerce_template_parms 
(on trunk), so you shouldn't need to do it again here.

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

Maybe another new tree code for constrained-type-specifier?

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

Probably.

Jason

Re: Concepts code review

[Andrew Sutton]

>> Agreed. I'll probably start looking at this on Friday morning.
>
>
> Note that end of stage 1 is Saturday, as I just realized today.  So the
> sooner the better.  :)

Ouch. Good thing my merge with trunk broke in unexpected ways this
morning -- minimally, something in cgraph ended up missing a #include
in the merge and I don't know how to fix it :/

Andrew

Re: Concepts code review

[Jason Merrill]

On 11/12/2014 06:11 PM, Andrew Sutton wrote:
>>> Agreed. I'll probably start looking at this on Friday morning.
>>
>>
>> Note that end of stage 1 is Saturday, as I just realized today.  So the
>> sooner the better.  :)
>
> Ouch. Good thing my merge with trunk broke in unexpected ways this
> morning -- minimally, something in cgraph ended up missing a #include
> in the merge and I don't know how to fix it :/

I sometimes get include problems when I update my sources and need to 
remove my build tree and build again from scratch.  Hopefully that's 
what you're seeing, too.

Jason

Re: Concepts code review

[Braden Obrzut]

[-- Attachment #1: Type: text/plain, Size: 5382 bytes --]

I don't believe I'll be able to familiarize myself adequately with the 
more complex issues before the stage 1 deadline (from what I understand 
Andrew is/was taking care of the blocking issues?), so I will leave what 
I have for the more trivial issues and a few comments.

On 11/11/2014 12:05 PM, Jason Merrill wrote:
>
>> // 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.
>
Variable templates (and thus concepts) are TEMPLATE_ID_EXPR.  I changed 
the comment to explicitly state that it will be a TEMPLATE_ID_EXPR, but 
I'm not sure the name needs to be changed.  If I recall correctly, I 
initially implemented as *_var_concept and Andrew told me to shorten 
it.  Note that this also matches up with normalize_var.
>
>> +// 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.
>
Even though Andrew said that this will change later for other reasons, 
it's a function I wrote so: I actually debated this with Andrew before.  
My rationale for calling begin_scope in the function was that it feels 
consistent with the semantics of the call. Specifically it can be seen 
as reopening the function parameter scope.  Thus the call is balanced by 
calling finish_scope.  Either way would work of course, but perhaps it 
just needed a better name and/or comment?
>
>> +      // 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.
>
Probably.  The instance you quoted isn't very trivial though.  The 
requirements are saved in two different branches and need to be restored 
before a certain point in the function.  Might just need to spend more 
time looking over the code.
>
>> +  // 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.
>
This is an interesting change.  The REQUIRES_EXPR is currently marked as 
value dependent.  The ChangeLog indicates that Andrew loosened the 
conditions for being value dependent for some cases, but then added it 
as type dependent when something else failed.  May require some time to 
pin down exactly what needs to be done here.

- Braden Obrzut

2014-11-15  Braden Obrzut  <admin@maniacsvault.net>

     * gcc/cp/constraint.cc (resolve_constraint_check): Move definition
     check to grokfndecl.
     (normalize_template_id): Use expression location if available when
     informing about missing parentheses.
     (build_requires_expr): Added comment.
     (diagnose_var): Clarified comment.
     * gcc/cp/decl.c (check_concept_refinement): Remove outdated comment
     regarding variable concepts.
     (grokfndecl): Ensure that all concept declarations are definitions.
     (grokdeclarator): Remove outdated comment regarding variable concepts.
     * gcc/cp/parser.c (cp_parser_introduction_list): Use vec for temporary
     list instead of a TREE_LIST.
     (get_id_declarator): Renamed from cp_get_id_declarator.
     (get_unqualified_id): Renamed from cp_get_identifier.
     (is_constrained_parameter): Renamed from cp_is_constrained_parameter.
     (cp_parser_check_constrained_type_parm): Renamed from
     cp_check_constrained_type_parm.
     (cp_parser_constrained_type_template_parm): Renamed from
     cp_constrained_type_template_parm.
     (cp_parser_constrained_template_template_parm): Renamed from
     cp_constrained_template_template_parm.
     (constrained_non_type_template_parm): Renamed from
     cp_constrained_non_type_tmeplate_parm.
     (finish_constrained_parameter): Renamed from
     cp_finish_constrained_parameter.
     (maybe_type_parameter): Renamed from cp_maybe_type_parameter.
     (declares_type_parameter): Renamed from cp_declares_type_parameter.
     (declares_type_template_parameter): Renamed from
     cp_declares_type_template_parameter.
     (declares_template_template_parameter): Renamed from
     cp_declares_template_template_parameter.
     (cp_parser_type_parameter): Call
     cp_parser_default_type_template_argument and
     cp_parser_default_template_template_argument which were already
     factored out from this function.
     (cp_maybe_constrained_type_specifier): Use the new INTRODUCED_PARM_DECL
     instead of PLACEHOLDER_EXPR.
     (cp_parser_requires_expr_scope): Remove old comment and change
     destructor to use pop_bindings_and_leave_scope.
     (cp_parser_requires_expression): Remove old comment.
     (get_concept_from_constraint): Remove old comment.
     * gcc/testsuite/g++.dg/concepts/decl-diagnose.C: Changed expected
     errors now that missing concept definitions are diagnosed earlier.


[-- Attachment #2: concept-review-fixes.diff --]
[-- Type: text/x-patch, Size: 21732 bytes --]

diff --git a/gcc/cp/constraint.cc b/gcc/cp/constraint.cc
index 2915b43..6550ec3 100644
--- a/gcc/cp/constraint.cc
+++ b/gcc/cp/constraint.cc
@@ -156,17 +156,6 @@ resolve_constraint_check (tree ovl, tree args)
   if (!DECL_DECLARED_CONCEPT_P (decl))
     return NULL_TREE;
 
-  // 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;
-    }
-
   return cands;
 }
 
@@ -610,13 +599,12 @@ normalize_template_id (tree t)
     return normalize_var (t);
   else
     {
-      // FIXME: input_location is probably wrong, but there's not necessarly
-      // an expr location with the tree.
-      error_at (input_location, "invalid constraint %qE", t);
+      location_t locus = EXPR_LOC_OR_LOC (t, input_location);
+      error_at (locus, "invalid constraint %qE", t);
 
       vec<tree, va_gc>* args = NULL;
       tree c = finish_call_expr (t, &args, true, false, 0);
-      inform (input_location, "did you mean %qE", c);
+      inform (locus, "did you mean %qE", c);
 
       return error_mark_node;
     }
@@ -865,6 +853,8 @@ valid_requirements_p (tree cinfo)
   return CI_ASSUMPTIONS (cinfo) != error_mark_node;
 }
 
+// Constructs a REQUIRES_EXPR with parameters, PARMS, and requirements, REQS,
+// that can be evaluated as a constant expression.
 tree
 build_requires_expr (tree parms, tree reqs)
 {
@@ -1836,7 +1826,7 @@ diagnose_call (location_t loc, tree t, tree args)
     inform (loc, "  %qE evaluated to false", t);
 }
 
-// Diagnose constraint failures in a variable concept.
+// Diagnose constraint failures in a variable template-id T.
 void
 diagnose_var (location_t loc, tree t, tree args)
 {
diff --git a/gcc/cp/decl.c b/gcc/cp/decl.c
index 0f9926c..bb635d8 100644
--- a/gcc/cp/decl.c
+++ b/gcc/cp/decl.c
@@ -1262,8 +1262,6 @@ check_concept_refinement (tree olddecl, tree newdecl)
   if (!DECL_DECLARED_CONCEPT_P (olddecl) || !DECL_DECLARED_CONCEPT_P (newdecl))
     return false;
 
-  // 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))
@@ -7724,6 +7722,13 @@ grokfndecl (tree ctype,
   // Was the concept specifier present?
   bool concept_p = inlinep & 4;
 
+  // Concept declarations must have a corresponding definition.
+  if (concept_p && !funcdef_flag)
+    {
+      error ("concept %qD has no definition", declarator);
+      return NULL_TREE;
+    }
+
   if (rqual)
     type = build_ref_qualified_type (type, rqual);
   if (raises)
@@ -11052,8 +11057,6 @@ grokdeclarator (const cp_declarator *declarator,
 		      DECL_GNU_TLS_P (decl) = true;
 		  }
 		if (concept_p)
-		  // TODO: This needs to be revisited once variable
-		  // templates are supported
 		    error ("static data member %qE declared %<concept%>",
 			   unqualified_id);
 		else if (constexpr_p && !initialized)
diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c
index 65d4acc..70bbbab 100644
--- a/gcc/cp/parser.c
+++ b/gcc/cp/parser.c
@@ -13344,7 +13344,7 @@ cp_parser_template_parameter_list (cp_parser* parser)
 static tree
 cp_parser_introduction_list (cp_parser *parser)
 {
-  tree introduction_list = NULL_TREE;
+  vec<tree, va_gc> *introduction_vec = make_tree_vector ();
 
   while (true)
     {
@@ -13358,8 +13358,7 @@ cp_parser_introduction_list (cp_parser *parser)
 	= cp_lexer_peek_token (parser->lexer)->location;
       DECL_NAME (parm) = cp_parser_identifier (parser);
       INTRODUCED_PACK_P (parm) = is_pack;
-      introduction_list = chainon (introduction_list,
-				   build_tree_list (NULL_TREE, parm));
+      introduction_vec->quick_push (parm);
 
       // If the next token is not a `,', we're done.
       if (cp_lexer_next_token_is_not (parser->lexer, CPP_COMMA))
@@ -13368,27 +13367,23 @@ cp_parser_introduction_list (cp_parser *parser)
       cp_lexer_consume_token (parser->lexer);
     }
 
-  // Convert the TREE_LIST into a TREE_VEC, similar to what is done in
-  // end_template_parm_list.
-  tree introduction_vec = make_tree_vec (list_length (introduction_list));
-  int n = 0;
-  while (introduction_list != NULL_TREE)
+  // Convert the vec into a TREE_VEC.
+  tree introduction_list = make_tree_vec (introduction_vec->length ());
+  unsigned int n;
+  tree parm;
+  FOR_EACH_VEC_ELT (*introduction_vec, n, parm)
     {
-       tree next = TREE_CHAIN (introduction_list);
-       TREE_VEC_ELT (introduction_vec, n) = TREE_VALUE (introduction_list);
-       TREE_CHAIN (introduction_list) = NULL_TREE;
-
-       introduction_list = next;
-       ++n;
+       TREE_VEC_ELT (introduction_list, n) = parm;
     }
 
-  return introduction_vec;
+  release_tree_vector (introduction_vec);
+  return introduction_list;
 }
 
 // Given a declarator, get the declarator-id part, or NULL_TREE if this
 // is an abstract declarator.
 static inline cp_declarator*
-cp_get_id_declarator (cp_declarator *declarator)
+get_id_declarator (cp_declarator *declarator)
 {
   cp_declarator *d = declarator;
   while (d && d->kind != cdk_id)
@@ -13399,9 +13394,9 @@ cp_get_id_declarator (cp_declarator *declarator)
 // Get the unqualified-id from the DECLARATOR or NULL_TREE if this
 // is an abstract declarator.
 static inline tree
-cp_get_identifier (cp_declarator *declarator)
+get_unqualified_id (cp_declarator *declarator)
 {
-  declarator = cp_get_id_declarator (declarator);
+  declarator = get_id_declarator (declarator);
   if (declarator)
     return declarator->u.id.unqualified_name;
   else
@@ -13410,7 +13405,7 @@ cp_get_identifier (cp_declarator *declarator)
 
 // Returns true if PARM declares a constrained-parameter.
 static inline bool
-cp_is_constrained_parameter (cp_parameter_declarator *parm)
+is_constrained_parameter (cp_parameter_declarator *parm)
 {
   gcc_assert (parm);
   tree decl = parm->decl_specifiers.type;
@@ -13426,8 +13421,8 @@ cp_is_constrained_parameter (cp_parameter_declarator *parm)
 // Check that the type parameter is only a declarator-id, and that its
 // type is not cv-qualified.
 bool
-cp_check_constrained_type_parm (cp_parser *parser,
-                                cp_parameter_declarator *parm)
+cp_parser_check_constrained_type_parm (cp_parser *parser,
+				       cp_parameter_declarator *parm)
 {
   if (!parm->declarator)
     return true;
@@ -13452,11 +13447,11 @@ cp_check_constrained_type_parm (cp_parser *parser,
 // Finish parsing/processing a template type parameter and chekcing
 // various restrictions.
 static inline tree
-cp_constrained_type_template_parm (cp_parser *parser,
-                                   tree id,
-                                   cp_parameter_declarator* parmdecl)
+cp_parser_constrained_type_template_parm (cp_parser *parser,
+                                          tree id,
+                                          cp_parameter_declarator* parmdecl)
 {
-  if (cp_check_constrained_type_parm (parser, parmdecl))
+  if (cp_parser_check_constrained_type_parm (parser, parmdecl))
     return finish_template_type_parm (class_type_node, id);
   else
     return error_mark_node;
@@ -13465,12 +13460,12 @@ cp_constrained_type_template_parm (cp_parser *parser,
 // Finish parsing/processing a template template parameter by borrowing
 // the template parameter list from the prototype parameter.
 static tree
-cp_constrained_template_template_parm (cp_parser *parser,
-                                       tree proto,
-                                       tree id,
-                                       cp_parameter_declarator *parmdecl)
+cp_parser_constrained_template_template_parm (cp_parser *parser,
+                                              tree proto,
+                                              tree id,
+                                              cp_parameter_declarator *parmdecl)
 {
-  if (!cp_check_constrained_type_parm (parser, parmdecl))
+  if (!cp_parser_check_constrained_type_parm (parser, parmdecl))
     return error_mark_node;
 
   // FIXME: This should probably be copied, and we may need to adjust
@@ -13487,8 +13482,8 @@ cp_constrained_template_template_parm (cp_parser *parser,
 
 // Create a new non-type template parameter from the given PARM declarator.
 static tree
-cp_constrained_non_type_template_parm (bool *is_non_type,
-                                       cp_parameter_declarator *parm)
+constrained_non_type_template_parm (bool *is_non_type,
+                                    cp_parameter_declarator *parm)
 {
   *is_non_type = true;
   cp_declarator *decl = parm->declarator;
@@ -13502,13 +13497,13 @@ cp_constrained_non_type_template_parm (bool *is_non_type,
 // refers to the prototype template parameter that ultimately
 // specifies the type of the declared parameter.
 static tree
-cp_finish_constrained_parameter (cp_parser *parser,
-                                 cp_parameter_declarator *parmdecl,
-                                 bool *is_non_type,
-                                 bool *is_parameter_pack)
+finish_constrained_parameter (cp_parser *parser,
+                              cp_parameter_declarator *parmdecl,
+                              bool *is_non_type,
+                              bool *is_parameter_pack)
 {
   tree decl = parmdecl->decl_specifiers.type;
-  tree id = cp_get_identifier (parmdecl->declarator);
+  tree id = get_unqualified_id (parmdecl->declarator);
   tree def = parmdecl->default_argument;
   tree proto = DECL_INITIAL (decl);
 
@@ -13521,11 +13516,12 @@ cp_finish_constrained_parameter (cp_parser *parser,
   // Build the parameter. Return an error if the declarator was invalid.
   tree parm;
   if (TREE_CODE (proto) == TYPE_DECL)
-    parm = cp_constrained_type_template_parm (parser, id, parmdecl);
+    parm = cp_parser_constrained_type_template_parm (parser, id, parmdecl);
   else if (TREE_CODE (proto) == TEMPLATE_DECL)
-    parm = cp_constrained_template_template_parm (parser, proto, id, parmdecl);
+    parm = cp_parser_constrained_template_template_parm (parser, proto, id,
+							 parmdecl);
   else
-    parm = cp_constrained_non_type_template_parm (is_non_type, parmdecl);
+    parm = constrained_non_type_template_parm (is_non_type, parmdecl);
   if (parm == error_mark_node)
     return error_mark_node;
 
@@ -13540,7 +13536,7 @@ cp_finish_constrained_parameter (cp_parser *parser,
 // a template type parameter. This is a helper function for the
 // cp_declares_type* functions below.
 static inline bool
-cp_maybe_type_parameter (tree type)
+maybe_type_parameter (tree type)
 {
   return type
          && TREE_CODE (type) == TYPE_DECL
@@ -13551,9 +13547,9 @@ cp_maybe_type_parameter (tree type)
 // Returns true if the parsed type actually represents a type or template
 // template parameter.
 static inline bool
-cp_declares_type_parameter (tree type)
+declares_type_parameter (tree type)
 {
-  if (cp_maybe_type_parameter (type))
+  if (maybe_type_parameter (type))
     {
       tree_code c = TREE_CODE (TREE_TYPE (type));
       return c == TEMPLATE_TYPE_PARM || c == TEMPLATE_TEMPLATE_PARM;
@@ -13564,9 +13560,9 @@ cp_declares_type_parameter (tree type)
 // Returns true if the parsed type actually represents the declaration
 // of a type template-parameter.
 static inline bool
-cp_declares_type_template_parameter (tree type)
+declares_type_template_parameter (tree type)
 {
-  return cp_maybe_type_parameter (type)
+  return maybe_type_parameter (type)
          && TREE_CODE (TREE_TYPE (type)) == TEMPLATE_TYPE_PARM;
 }
 
@@ -13574,9 +13570,9 @@ cp_declares_type_template_parameter (tree type)
 // Returns true if the parsed type actually represents the declaration of
 // a template template-parameter.
 static bool
-cp_declares_template_template_parameter (tree type)
+declares_template_template_parameter (tree type)
 {
-  return cp_maybe_type_parameter (type)
+  return maybe_type_parameter (type)
          && TREE_CODE (TREE_TYPE (type)) == TEMPLATE_TEMPLATE_PARM;
 }
 
@@ -13784,20 +13780,20 @@ cp_parser_template_parameter (cp_parser* parser, bool *is_non_type,
 		  "template parameter pack cannot have a default argument");
       
       /* Parse the default argument, but throw away the result.  */
-      if (cp_declares_type_template_parameter (declared_type))
+      if (declares_type_template_parameter (declared_type))
         cp_parser_default_type_template_argument (parser);
-      else if (cp_declares_template_template_parameter (declared_type))
+      else if (declares_template_template_parameter (declared_type))
         cp_parser_default_template_template_argument (parser);
       else
         cp_parser_default_argument (parser, /*template_parm_p=*/true);
     }
 
   // The parameter may have been constrained.
-  if (cp_is_constrained_parameter (parameter_declarator))
-    return cp_finish_constrained_parameter (parser,
-                                            parameter_declarator,
-                                            is_non_type,
-                                            is_parameter_pack);
+  if (is_constrained_parameter (parameter_declarator))
+    return finish_constrained_parameter (parser,
+                                         parameter_declarator,
+                                         is_non_type,
+                                         is_parameter_pack);
 
   // Now we're sure that the parameter is a non-type parameter.
   *is_non_type = true;
@@ -13878,11 +13874,8 @@ cp_parser_type_parameter (cp_parser* parser, bool *is_parameter_pack)
 	/* If the next token is an `=', we have a default argument.  */
 	if (cp_lexer_next_token_is (parser->lexer, CPP_EQ))
 	  {
-	    /* Consume the `=' token.  */
-	    cp_lexer_consume_token (parser->lexer);
-	    /* Parse the default-argument.  */
-	    push_deferring_access_checks (dk_no_deferred);
-	    default_argument = cp_parser_type_id (parser);
+	    default_argument
+	      = cp_parser_default_type_template_argument (parser);
 
             /* Template parameter packs cannot have default
                arguments. */
@@ -13898,7 +13891,6 @@ cp_parser_type_parameter (cp_parser* parser, bool *is_parameter_pack)
 			    "default arguments");
                 default_argument = NULL_TREE;
               }
-	    pop_deferring_access_checks ();
 	  }
 	else
 	  default_argument = NULL_TREE;
@@ -13977,40 +13969,8 @@ cp_parser_type_parameter (cp_parser* parser, bool *is_parameter_pack)
 	   default-argument.  */
 	if (cp_lexer_next_token_is (parser->lexer, CPP_EQ))
 	  {
-	    bool is_template;
-
-	    /* Consume the `='.  */
-	    cp_lexer_consume_token (parser->lexer);
-	    /* Parse the id-expression.  */
-	    push_deferring_access_checks (dk_no_deferred);
-	    /* save token before parsing the id-expression, for error
-	       reporting */
-	    token = cp_lexer_peek_token (parser->lexer);
-	    default_argument
-	      = cp_parser_id_expression (parser,
-					 /*template_keyword_p=*/false,
-					 /*check_dependency_p=*/true,
-					 /*template_p=*/&is_template,
-					 /*declarator_p=*/false,
-					 /*optional_p=*/false);
-	    if (TREE_CODE (default_argument) == TYPE_DECL)
-	      /* If the id-expression was a template-id that refers to
-		 a template-class, we already have the declaration here,
-		 so no further lookup is needed.  */
-		 ;
-	    else
-	      /* Look up the name.  */
-	      default_argument
-		= cp_parser_lookup_name (parser, default_argument,
-					 none_type,
-					 /*is_template=*/is_template,
-					 /*is_namespace=*/false,
-					 /*check_dependency=*/true,
-					 /*ambiguous_decls=*/NULL,
-					 token->location);
-	    /* See if the default argument is valid.  */
 	    default_argument
-	      = check_template_template_default_arg (default_argument);
+	      = cp_parser_default_template_template_argument (parser);
 
             /* Template parameter packs cannot have default
                arguments. */
@@ -14026,7 +13986,6 @@ cp_parser_type_parameter (cp_parser* parser, bool *is_parameter_pack)
 			    "have default arguments");
                 default_argument = NULL_TREE;
               }
-	    pop_deferring_access_checks ();
 	  }
 	else
 	  default_argument = NULL_TREE;
@@ -15586,7 +15545,7 @@ cp_parser_type_name (cp_parser* parser)
 	  && TREE_CODE (type_decl) == TYPE_DECL
 	  && TYPE_DECL_ALIAS_P (type_decl))
 	gcc_assert (DECL_TEMPLATE_INSTANTIATION (type_decl));
-      else if (cp_maybe_type_parameter (type_decl))
+      else if (maybe_type_parameter (type_decl))
         /* Don't do anything. */ ;
       else
 	cp_parser_simulate_error (parser);
@@ -15639,7 +15598,7 @@ cp_maybe_constrained_type_specifier (cp_parser *parser, tree decl, tree args)
 
   // Try to build a call expression that evaluates the concept. This
   // can fail if the overload set refers only to non-templates.
-  tree placeholder = build_nt (PLACEHOLDER_EXPR);
+  tree placeholder = build_nt (INTRODUCED_PARM_DECL);
   tree call = build_concept_check (decl, placeholder, args);
   if (call == error_mark_node)
     return NULL_TREE;
@@ -19639,12 +19598,12 @@ cp_parser_parameter_declaration (cp_parser *parser,
 	default_argument = cp_parser_cache_defarg (parser, /*nsdmi=*/false);
 
       // A constrained-type-specifier may declare a type template-parameter.
-      else if (cp_declares_type_template_parameter (type))
+      else if (declares_type_template_parameter (type))
         default_argument
           = cp_parser_default_type_template_argument (parser);
 
       // A constrained-type-specifier may declare a template-template-parameter.
-      else if (cp_declares_template_template_parameter (type))
+      else if (declares_template_template_parameter (type))
         default_argument
           = cp_parser_default_template_template_argument (parser);
 
@@ -23311,7 +23270,6 @@ cp_parser_requires_clause_opt (cp_parser *parser)
 // within the scope are popped prior to exiting the scope.
 struct cp_parser_requires_expr_scope
 {
-  // Enter a scope of kind K belonging to the decl D.
   cp_parser_requires_expr_scope ()
   {
     begin_scope (sk_block, NULL_TREE);
@@ -23319,9 +23277,7 @@ struct cp_parser_requires_expr_scope
 
   ~cp_parser_requires_expr_scope ()
   {
-    for (tree t = current_binding_level->names; t; t = DECL_CHAIN (t))
-      pop_binding (DECL_NAME (t), t);
-    leave_scope ();
+    pop_bindings_and_leave_scope ();
   }
 };
 
@@ -23345,10 +23301,6 @@ cp_parser_requires_expression (cp_parser *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.
-
   // Parse the optional parameter list. Any local parameter declarations
   // are added to a new scope and are visible within the nested
   // requirement list.
@@ -33692,13 +33644,6 @@ tree_type_is_auto_or_concept (const_tree t)
 
 // Return the template decl being called or evaluated as part of the
 // constraint check.
-//
-// 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.
 static tree
 get_concept_from_constraint (tree t)
 {
diff --git a/gcc/cp/parser.h b/gcc/cp/parser.h
index 6e57a72..ee6e2b2 100644
--- a/gcc/cp/parser.h
+++ b/gcc/cp/parser.h
@@ -397,9 +397,9 @@ typedef struct GTY(()) cp_parser {
      member definition using a generic type, it is the sk_class scope.  */
   cp_binding_level* implicit_template_scope;
 
-  /* True if parsing a template parameter list. The interpretation of a
-     constrained-type-specifiers differs inside a template parameter
-     list than outside. */
+  /* True if parsing a result type in a compound requirement. This permits
+     constrained-type-specifiers inside what would normally be a trailing
+     return type. */
   bool in_result_type_constraint_p;
 
 } cp_parser;
diff --git a/gcc/testsuite/g++.dg/concepts/decl-diagnose.C b/gcc/testsuite/g++.dg/concepts/decl-diagnose.C
index d3374f9..77fc40d 100644
--- a/gcc/testsuite/g++.dg/concepts/decl-diagnose.C
+++ b/gcc/testsuite/g++.dg/concepts/decl-diagnose.C
@@ -4,14 +4,17 @@ typedef concept int CINT; // { dg-error "'concept' cannot appear in a typedef de
 
 void f(concept int); // { dg-error "a parameter cannot be declared 'concept'" }
 
-concept int f2(); // { dg-error "return type" }
-concept bool f3();
+template<typename T>
+concept int f2() { return 0; } // { dg-error "return type" }
+concept bool f3(); // { dg-error "no definition" }
 
 struct X
 {
-  concept int f4(); // { dg-error "return type|function parameters" }
-  concept bool f5(); // { dg-error "declared with function parameters" }
-  static concept bool f6(); // { dg-error "a concept cannot be a static member function" }
+  template<typename T>
+  concept int f4() { return 0; } // { dg-error "return type|function parameters" }
+  concept bool f5() { return true; } // { dg-error "declared with function parameters" }
+  template<typename T>
+  static concept bool f6() { return true; } // { dg-error "a concept cannot be a static member function" }
   static concept bool x; // { dg-error "declared 'concept'" }
   concept int x2; // { dg-error "declared 'concept'" }
   concept ~X(); // { dg-error "a destructor cannot be 'concept'" }

Re: Concepts code review

[Jason Merrill]

On 11/15/2014 07:58 PM, Braden Obrzut wrote:
> Variable templates (and thus concepts) are TEMPLATE_ID_EXPR.  I changed
> the comment to explicitly state that it will be a TEMPLATE_ID_EXPR, but
> I'm not sure the name needs to be changed.  If I recall correctly, I
> initially implemented as *_var_concept and Andrew told me to shorten
> it.  Note that this also matches up with normalize_var.

OK.

>> I think if the caller is calling finish_scope, I'd prefer for the
>> begin_scope call to be there as well.
>>
> Even though Andrew said that this will change later for other reasons,
> it's a function I wrote so: I actually debated this with Andrew before.
> My rationale for calling begin_scope in the function was that it feels
> consistent with the semantics of the call. Specifically it can be seen
> as reopening the function parameter scope.  Thus the call is balanced by
> calling finish_scope.  Either way would work of course, but perhaps it
> just needed a better name and/or comment?

A better name would be OK.  Perhaps push_function_parms_with_scope.

Jason

Re: Concepts code review

[Andrew Sutton]

>>> +// 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.
>>
> Even though Andrew said that this will change later for other reasons, it's
> a function I wrote so: I actually debated this with Andrew before.  My
> rationale for calling begin_scope in the function was that it feels
> consistent with the semantics of the call. Specifically it can be seen as
> reopening the function parameter scope.  Thus the call is balanced by
> calling finish_scope.  Either way would work of course, but perhaps it just
> needed a better name and/or comment?

In the process of removing constraints from lang_decl nodes, I also
ended up addressing a lot of the other constraint processing comments
-- it made sense to do it at the same time.

One of those was moving a requires-clause into a function declarator.
I had thought that this would prevent me from having to re-open that
scope, but it doesn't. The parameter scope is closed at a lower level
in the parse :/

So this issue is still around.

>>> +      // 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.
>>
> Probably.  The instance you quoted isn't very trivial though.  The
> requirements are saved in two different branches and need to be restored
> before a certain point in the function.  Might just need to spend more time
> looking over the code.

I got rid of current_template_reqs in my current work. Constraints are
associated directly with a template parameter list or a declarator.

>>> +  // 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.
>>
> This is an interesting change.  The REQUIRES_EXPR is currently marked as
> value dependent.  The ChangeLog indicates that Andrew loosened the
> conditions for being value dependent for some cases, but then added it as
> type dependent when something else failed.  May require some time to pin
> down exactly what needs to be done here.

I think something may have changed since I made that change... If I'm
remembering correctly, it used to be the case that build_x_binary_op
would try to fold the resulting expression when the operands weren't
type dependent. That happens in conjoin_constraints.

Now it looks like it's calling build_non_dependent_expr, which does
something a little different.

I agree that requires-expressions are not type-dependent (they have type bool).

Andrew