This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Concepts code review
- From: Jason Merrill <jason at redhat dot com>
- To: Andrew Sutton <andrew dot n dot sutton at gmail dot com>
- Cc: Braden Obrzut <admin at maniacsvault dot net>, gcc-patches List <gcc-patches at gcc dot gnu dot org>
- Date: Tue, 11 Nov 2014 12:05:53 -0500
- Subject: Concepts code review
- Authentication-results: sourceware.org; auth=none
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
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);
-}
-
/* 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);