Re: [c++-concepts] code review
Today is the first day I've had to look at these comments.
>>if (TEMPLATE_PARM_CONSTRAINTS (current_template_parms))
>> -TYPE_CANONICAL (type) = type;
>> +SET_TYPE_STRUCTURAL_EQUALITY (type);
>
>
> This seems like papering over an underlying issue. What was the testcase
> that motivated this change?
It almost certainly is, but I haven't been able to find or write a
minimal test case that reproduces the reason for failure. Basically,
we end up with multiple specializations having the same type but
different constraints (since constraints are attached to the
declaration and not the type itself).
I think that I'm running into the same problems with auto a
placeholder in recent commits.
>> @@ -11854,7 +11854,7 @@ tsubst_decl (tree t, tree args, tsubst_flags_t
>> complain)
>> - if (!spec)
>> + if (!spec && DECL_LANG_SPECIFIC (t))
>> - if (!local_p)
>> + if (!local_p && DECL_LANG_SPECIFIC (r))
>
>
> What motivated these changes? From the testcase, it seems that you're
> getting here with the decl for "using TD = int", which shouldn't happen.
That's the pretty much it... I suppose we could guard against
substituting into these kinds of declarations from within
tsubst_type_requirement and satisfy_type_constraint. To me it seems
like tsubst should work, but just return the same thing.
>> @@ -1159,7 +1159,6 @@ check_noexcept_r (tree *tp, int * /*walk_subtrees*/,
>> void * /*data*/)
>> - tree type = TREE_TYPE (TREE_TYPE (fn));
>> - if (!TYPE_NOTHROW_P (type))
>> + if (!TYPE_NOTHROW_P (TREE_TYPE (fn)))
>
>
> The old code was incorrectly assuming that CALL_EXPR_FN is always a function
> pointer, but your new code seems to be incorrectly assuming that it's always
> a function or an expression taking the address of a function; I think this
> will break on a call to a function pointer variable.
I will experiment.
>> @@ -3481,13 +3481,27 @@ cxx_eval_constant_expression (const constexpr_ctx
>> *ctx, tree t,
>> case REQUIRES_EXPR:
>> + if (!processing_template_decl)
>> +return evaluate_constraint_expression (t, NULL_TREE);
>> + else
>> +*non_constant_p = true;
>> +return t;
>
>
> We shouldn't get here with a dependent REQUIRES_EXPR (or any dependent
> expression), so we shouldn't ever hit the else clause.
IIRC we get here because of build_x_binary_op. It tries to build
non-dependent operands when the operands are not type-dependent.
requires-expressions have type bool, so they get run through the
constexpr evaluator even when processing_template_decl is true.
I've made requires-expressions instantiation dependent, but that
doesn't help in this case.
>> +static inline bool
>> +pending_expansion_p (tree t)
>> +{
>> + return (TREE_CODE (t) == PARM_DECL && CONSTRAINT_VAR_P (t)
>> + && PACK_EXPANSION_P (TREE_TYPE (t)));
>> +}
>
>
> What's the difference between this and function_parameter_pack_p?
Not a lot, except that replacing pending_expansion_p in one of the two
places that it's used causes ICEs :) This function can almost
certainly be removed.
>> +check_implicit_conversion_constraint (tree t, tree args,
>> + tsubst_flags_t complain, tree
>> in_decl)
>> +{
>> + tree expr = ICONV_CONSTR_EXPR (t);
>> +
>> + /* Don't tsubst as if we're processing a template. If we try
>> + to we can end up generating template-like expressions
>> + (e.g., modop-exprs) that aren't properly typed. */
>> + int saved_template_decl = processing_template_decl;
>> + processing_template_decl = 0;
>
>
> Why are we checking constraints when processing_template_decl is true?
IIRC I allow constraints to be evaluated in any context because it
lets us catch these kinds of errors:
template
void f()
{
vector v; // error: constraints not satisfied
}
>> + ++processing_template_decl;
>> + tree constr = transform_expression (lift_function_definition (fn,
>> args));
>> + --processing_template_decl;
>
>
> Why do you need to set processing_template_decl here and in other calls to
> transform_expression? I don't notice anything that would be helped,
> especially now that you're using separate tree codes for constraints, though
> there is this in check_logical_expr:
>
>> + /* Resolve the logical operator. Note that template processing is
>> + disabled so we get the actual call or target expression back.
>> + not_processing_template_sentinel sentinel.
>
>
> I guess that isn't needed anymore?
I've had problems in the past where substitution tries a little too
eagerly to fold expressions into constants --- especially type traits.
Those need to be preserved in the text for ordering.
Although I think this really only matters when you're instantiating a
class template whose members are constraints.
Andrew
Re: [c++-concepts] code review
> It looks like things are coming together pretty well. What's your feeling
> about readiness to merge into the trunk? Is the branch down to no
> regressions?
They are coming together pretty well. We have one major unit test
failure involving template introductions (Braden is working on it),
one involving constraint equivalence that I plan to tackle next week.
Other than those issues, which I hope to clear up next week, I think it's ready.
> See you on Monday!
Unfortunately, I won't be attending.
Andrew
>
>> @@ -4146,21 +4146,21 @@ build_new_function_call (tree fn, vec
>> **args, bool koenig_p,
>>if (TREE_CODE (fn) == TEMPLATE_ID_EXPR)
>> {
>>/* If overload resolution selects a specialization of a
>> + function concept for non-dependent template arguments,
>> + the expression is true if the constraints are satisfied
>> + and false otherwise.
>>
>> NOTE: This is an extension of Concepts Lite TS that
>> allows constraints to be used in expressions. */
>> + if (flag_concepts && !processing_template_decl)
>> {
>>tree tmpl = DECL_TI_TEMPLATE (cand->fn);
>> + tree targs = DECL_TI_ARGS (cand->fn);
>>tree decl = DECL_TEMPLATE_RESULT (tmpl);
>> + if (DECL_DECLARED_CONCEPT_P (decl)
>> + && !uses_template_parms (targs)) {
>> +return evaluate_function_concept (decl, targs);
>
>
> If processing_template_decl is false, uses_template_parms should always be
> false as well.
>
>> +function_concept_check_p (tree t)
>
>
>> + tree fn = CALL_EXPR_FN (t);
>> + if (TREE_CODE (fn) == TEMPLATE_ID_EXPR
>> + && TREE_CODE (TREE_OPERAND (fn, 0)) == OVERLOAD)
>> +{
>> + tree f1 = OVL_FUNCTION (TREE_OPERAND (fn, 0));
>
>
> I think you want get_first_fn here.
>
>>if (TEMPLATE_PARM_CONSTRAINTS (current_template_parms))
>> -TYPE_CANONICAL (type) = type;
>> +SET_TYPE_STRUCTURAL_EQUALITY (type);
>
>
> This seems like papering over an underlying issue. What was the testcase
> that motivated this change?
>
>> @@ -11854,7 +11854,7 @@ tsubst_decl (tree t, tree args, tsubst_flags_t
>> complain)
>> - if (!spec)
>> + if (!spec && DECL_LANG_SPECIFIC (t))
>> - if (!local_p)
>> + if (!local_p && DECL_LANG_SPECIFIC (r))
>
>
> What motivated these changes? From the testcase, it seems that you're
> getting here with the decl for "using TD = int", which shouldn't happen.
>
>> @@ -1159,7 +1159,6 @@ check_noexcept_r (tree *tp, int * /*walk_subtrees*/,
>> void * /*data*/)
>> - tree type = TREE_TYPE (TREE_TYPE (fn));
>> - if (!TYPE_NOTHROW_P (type))
>> + if (!TYPE_NOTHROW_P (TREE_TYPE (fn)))
>
>
> The old code was incorrectly assuming that CALL_EXPR_FN is always a function
> pointer, but your new code seems to be incorrectly assuming that it's always
> a function or an expression taking the address of a function; I think this
> will break on a call to a function pointer variable.
>
>> @@ -3481,13 +3481,27 @@ cxx_eval_constant_expression (const constexpr_ctx
>> *ctx, tree t,
>> case REQUIRES_EXPR:
>> + if (!processing_template_decl)
>> +return evaluate_constraint_expression (t, NULL_TREE);
>> + else
>> +*non_constant_p = true;
>> +return t;
>
>
> We shouldn't get here with a dependent REQUIRES_EXPR (or any dependent
> expression), so we shouldn't ever hit the else clause.
>
>> @@ -18063,18 +18063,41 @@ cp_parser_declarator (cp_parser* parser,
>> + /* Function declarations may be followed by a trailing
>> + requires-clause. Declarators for function declartions
>> + are function declarators wrapping an id-declarator.
>> + If the inner declarator is anything else, it does not
>> + declare a function. These may also be reference or
>> + pointer declarators enclosing such a function declarator.
>> + In the declaration :
>> +
>> +int *f(args)
>> +
>> + the declarator is *f(args).
>> +
>> + Abstract declarators cannot have a requires-clauses
>> + because they do not declare functions. Here:
>>
>> void f() -> int& requires false
>>
>> + The trailing return type contains an abstract declarator,
>> + and the requires-clause applies to the function
>> + declaration and not the abstract declarator. */
>> + if (flag_concepts && dcl_kind != CP_PARSER_DECLARATOR_ABSTRACT)
>> {
>> + /* We could have things like *f(args) or &f(args).
>> + Look inside references and pointers. */
>> + cp_declarator* p = declarator;
>> + if (p->kind == cdk_reference || p->kind == cdk_pointer)
>> +p = p->declarator;
>> +
>> + /* Pointers or references with no name, or functions
>> + with no name cannot have constraints. */
>> + if (!p || !p->declarator)
>> +return declarator;
>> +
>> + /* Look for f(args) but not (*f)(args). */
>> +
[c++-concepts] code review
It looks like things are coming together pretty well. What's your
feeling about readiness to merge into the trunk? Is the branch down to
no regressions?
See you on Monday!
@@ -4146,21 +4146,21 @@ build_new_function_call (tree fn, vec
**args, bool koenig_p,
if (TREE_CODE (fn) == TEMPLATE_ID_EXPR)
{
/* If overload resolution selects a specialization of a
+ function concept for non-dependent template arguments,
+ the expression is true if the constraints are satisfied
+ and false otherwise.
NOTE: This is an extension of Concepts Lite TS that
allows constraints to be used in expressions. */
+ if (flag_concepts && !processing_template_decl)
{
tree tmpl = DECL_TI_TEMPLATE (cand->fn);
+ tree targs = DECL_TI_ARGS (cand->fn);
tree decl = DECL_TEMPLATE_RESULT (tmpl);
+ if (DECL_DECLARED_CONCEPT_P (decl)
+ && !uses_template_parms (targs)) {
+return evaluate_function_concept (decl, targs);
If processing_template_decl is false, uses_template_parms should always
be false as well.
+function_concept_check_p (tree t)
+ tree fn = CALL_EXPR_FN (t);
+ if (TREE_CODE (fn) == TEMPLATE_ID_EXPR
+ && TREE_CODE (TREE_OPERAND (fn, 0)) == OVERLOAD)
+{
+ tree f1 = OVL_FUNCTION (TREE_OPERAND (fn, 0));
I think you want get_first_fn here.
if (TEMPLATE_PARM_CONSTRAINTS (current_template_parms))
-TYPE_CANONICAL (type) = type;
+SET_TYPE_STRUCTURAL_EQUALITY (type);
This seems like papering over an underlying issue. What was the
testcase that motivated this change?
@@ -11854,7 +11854,7 @@ tsubst_decl (tree t, tree args, tsubst_flags_t complain)
- if (!spec)
+ if (!spec && DECL_LANG_SPECIFIC (t))
- if (!local_p)
+ if (!local_p && DECL_LANG_SPECIFIC (r))
What motivated these changes? From the testcase, it seems that you're
getting here with the decl for "using TD = int", which shouldn't happen.
@@ -1159,7 +1159,6 @@ check_noexcept_r (tree *tp, int * /*walk_subtrees*/, void
* /*data*/)
- tree type = TREE_TYPE (TREE_TYPE (fn));
- if (!TYPE_NOTHROW_P (type))
+ if (!TYPE_NOTHROW_P (TREE_TYPE (fn)))
The old code was incorrectly assuming that CALL_EXPR_FN is always a
function pointer, but your new code seems to be incorrectly assuming
that it's always a function or an expression taking the address of a
function; I think this will break on a call to a function pointer variable.
@@ -3481,13 +3481,27 @@ cxx_eval_constant_expression (const constexpr_ctx *ctx,
tree t,
case REQUIRES_EXPR:
+ if (!processing_template_decl)
+return evaluate_constraint_expression (t, NULL_TREE);
+ else
+*non_constant_p = true;
+return t;
We shouldn't get here with a dependent REQUIRES_EXPR (or any dependent
expression), so we shouldn't ever hit the else clause.
@@ -18063,18 +18063,41 @@ cp_parser_declarator (cp_parser* parser,
+ /* Function declarations may be followed by a trailing
+ requires-clause. Declarators for function declartions
+ are function declarators wrapping an id-declarator.
+ If the inner declarator is anything else, it does not
+ declare a function. These may also be reference or
+ pointer declarators enclosing such a function declarator.
+ In the declaration :
+
+int *f(args)
+
+ the declarator is *f(args).
+
+ Abstract declarators cannot have a requires-clauses
+ because they do not declare functions. Here:
void f() -> int& requires false
+ The trailing return type contains an abstract declarator,
+ and the requires-clause applies to the function
+ declaration and not the abstract declarator. */
+ if (flag_concepts && dcl_kind != CP_PARSER_DECLARATOR_ABSTRACT)
{
+ /* We could have things like *f(args) or &f(args).
+ Look inside references and pointers. */
+ cp_declarator* p = declarator;
+ if (p->kind == cdk_reference || p->kind == cdk_pointer)
+p = p->declarator;
+
+ /* Pointers or references with no name, or functions
+ with no name cannot have constraints. */
+ if (!p || !p->declarator)
+return declarator;
+
+ /* Look for f(args) but not (*f)(args). */
+ if (p && p->kind == cdk_function && p->declarator->kind == cdk_id)
I think you can use function_declarator_p here.
+static inline bool
+pending_expansion_p (tree t)
+{
+ return (TREE_CODE (t) == PARM_DECL && CONSTRAINT_VAR_P (t)
+ && PACK_EXPANSION_P (TREE_TYPE (t)));
+}
What's the difference between this and function_parameter_pack_p?
+ /* A sentinel class that ensures that deferred access checks
+ are popped before a function returns. */
+ struct deferring_access_check_sentinel
+ {
+deferring_access_check_sentinel ()
+{
+ push_deferring_access_checks (dk_deferre
Re: Concepts code review
>>> +// 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
Re: Concepts code review
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
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 * 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_
Re: Concepts code review
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
>> 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
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
concept bool C() { return true; }
template
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
>> + // 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'
Concepts code review
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 ("constex
Re: [c++-concepts] code review
On 06/21/2013 08:46 AM, Andrew Sutton wrote: I can move those patches over to git and push the changes in separate branches in addition to the usual submission mechanism. Would that be appropriate? Can I create a bunch of different git branches for small feature sets? Sure, that sounds fine. Jason
Re: [c++-concepts] code review
I think I will continue to work from SVN branches, because I'm a lot more familiar with that process. I'm also going to start working on a couple of different checkouts of the c++-concepts branch independently, so this should be a little easier for me. I can move those patches over to git and push the changes in separate branches in addition to the usual submission mechanism. Would that be appropriate? Can I create a bunch of different git branches for small feature sets? Andrew On Thu, Jun 20, 2013 at 1:33 PM, Jason Merrill wrote: > On 06/20/2013 01:23 PM, Jason Merrill wrote: >> >> Since Gaby prefers SVN, let's keep using the SVN branch; it really isn't >> much less convenient than a git-only branch. The main difference is >> 'git svn rebase'/'git svn dcommit' instead of 'git pull'/'git push'. > > > The one caveat is that git-svn historically hasn't handled merges very well. > I think git-svn v1.8.3 or newer can do the right thing, but I haven't tested > it, so it's probably best to keep doing merges with the svn client for the > time being. > > Jason > -- Andrew Sutton andrew.n.sut...@gmail.com
Re: [c++-concepts] code review
On 06/20/2013 01:23 PM, Jason Merrill wrote: Since Gaby prefers SVN, let's keep using the SVN branch; it really isn't much less convenient than a git-only branch. The main difference is 'git svn rebase'/'git svn dcommit' instead of 'git pull'/'git push'. The one caveat is that git-svn historically hasn't handled merges very well. I think git-svn v1.8.3 or newer can do the right thing, but I haven't tested it, so it's probably best to keep doing merges with the svn client for the time being. Jason
Re: [c++-concepts] code review
On 06/20/2013 11:50 AM, Andrew Sutton wrote: The c++-concepts SVN branch won't come in with a clone; you need to add it to the fetch list with git config --add remote.origin.fetch refs/remotes/c++-concepts:refs/remotes/origin/c++-concepts Then you can check out a local branch based on it. Already did that. I probably need to do this, right? git checkout -b c++-concepts origin/c++-concepts Right. I just setup using a git-only branch, so the latter. I set up to push changes to asutton/c++-concepts, so submitted should be visible there. Since Gaby prefers SVN, let's keep using the SVN branch; it really isn't much less convenient than a git-only branch. The main difference is 'git svn rebase'/'git svn dcommit' instead of 'git pull'/'git push'. Out of curiosity, how will reviews work? Just walk through the most recently pushed changes on my branch? That's what I was thinking, yes. I should be able to produce diffs to send to Gaby also. I should also be able to use those to patch an SVN checkout and commit from a different repo. git-svn is much more convenient :) Jason
Re: [c++-concepts] code review
> The c++-concepts SVN branch won't come in with a clone; you need to add it > to the fetch list with > > git config --add remote.origin.fetch > refs/remotes/c++-concepts:refs/remotes/origin/c++-concepts > > Then you can check out a local branch based on it. Already did that. I probably need to do this, right? git checkout -b c++-concepts origin/c++-concepts >> How will this change the workflow? What's the process for submitting >> changes using git? > > > That depends whether you want to keep it as an SVN branch or switch to a > git-only branch. Both processes are documented in the wiki page: > > http://gcc.gnu.org/wiki/GitMirror#Commit_upstream_.28git-svn.29 > http://gcc.gnu.org/wiki/GitMirror#Git-only_branches > > If it's unclear, please let me know so I can update the page. I just setup using a git-only branch, so the latter. I set up to push changes to asutton/c++-concepts, so submitted should be visible there. Out of curiosity, how will reviews work? Just walk through the most recently pushed changes on my branch? I should be able to produce diffs to send to Gaby also. I should also be able to use those to patch an SVN checkout and commit from a different repo. Andrew
Re: [c++-concepts] code review
Jason Merrill writes: | On 06/20/2013 11:22 AM, Gabriel Dos Reis wrote: | > Jason Merrill writes: | > | > | On 06/20/2013 10:17 AM, Andrew Sutton wrote: | > | > It looks like I should be able to switch directly to the c++-concepts | > | > branch. Or is there some configuration that has to happen on the | > | > remote site? | > | | > | The c++-concepts SVN branch won't come in with a clone; you need to | > | add it to the fetch list with | > | | > | git config --add remote.origin.fetch | > | refs/remotes/c++-concepts:refs/remotes/origin/c++-concepts | > | | > | Then you can check out a local branch based on it. | > | > Does that mean I also need to switch to git in order to continue | > maintaining the branch? I would rather stay with SVN for now. | | No, the above is just how you check out the SVN branch under git. Great! Thanks for the clarification. -- Gaby
Re: [c++-concepts] code review
On 06/20/2013 11:22 AM, Gabriel Dos Reis wrote: Jason Merrill writes: | On 06/20/2013 10:17 AM, Andrew Sutton wrote: | > It looks like I should be able to switch directly to the c++-concepts | > branch. Or is there some configuration that has to happen on the | > remote site? | | The c++-concepts SVN branch won't come in with a clone; you need to | add it to the fetch list with | | git config --add remote.origin.fetch | refs/remotes/c++-concepts:refs/remotes/origin/c++-concepts | | Then you can check out a local branch based on it. Does that mean I also need to switch to git in order to continue maintaining the branch? I would rather stay with SVN for now. No, the above is just how you check out the SVN branch under git. I prefer an SVN branch for now. OK. Jason
Re: [c++-concepts] code review
Jason Merrill writes: | On 06/20/2013 10:17 AM, Andrew Sutton wrote: | > It looks like I should be able to switch directly to the c++-concepts | > branch. Or is there some configuration that has to happen on the | > remote site? | | The c++-concepts SVN branch won't come in with a clone; you need to | add it to the fetch list with | | git config --add remote.origin.fetch | refs/remotes/c++-concepts:refs/remotes/origin/c++-concepts | | Then you can check out a local branch based on it. Does that mean I also need to switch to git in order to continue maintaining the branch? I would rather stay with SVN for now. | > How will this change the workflow? What's the process for submitting | > changes using git? | | That depends whether you want to keep it as an SVN branch or switch to | a git-only branch. Both processes are documented in the wiki page: I prefer an SVN branch for now. | | http://gcc.gnu.org/wiki/GitMirror#Commit_upstream_.28git-svn.29 | http://gcc.gnu.org/wiki/GitMirror#Git-only_branches | | If it's unclear, please let me know so I can update the page. | | Jason -- Gaby
Re: [c++-concepts] code review
Andrew Sutton writes: | That works. I think the current patch addresses all of Jason's comments. OK, that sounds good. | I'll also create a github version of this branch, so can avoid email patches. Well, actually I prefer the email patches because I can read them them right away when I have only email connections (yes, I am one those dinausors who still read emails with emacs remotely from a terminal.) I wish I had the luxury of organizing everything around browsers, but I don't. -- Gaby
Re: [c++-concepts] code review
On 06/20/2013 10:17 AM, Andrew Sutton wrote: It looks like I should be able to switch directly to the c++-concepts branch. Or is there some configuration that has to happen on the remote site? The c++-concepts SVN branch won't come in with a clone; you need to add it to the fetch list with git config --add remote.origin.fetch refs/remotes/c++-concepts:refs/remotes/origin/c++-concepts Then you can check out a local branch based on it. How will this change the workflow? What's the process for submitting changes using git? That depends whether you want to keep it as an SVN branch or switch to a git-only branch. Both processes are documented in the wiki page: http://gcc.gnu.org/wiki/GitMirror#Commit_upstream_.28git-svn.29 http://gcc.gnu.org/wiki/GitMirror#Git-only_branches If it's unclear, please let me know so I can update the page. Jason
Re: [c++-concepts] code review
I didn't know it existed! Even better. Except that I'm not a git expert. I'm reading through the docs on that site while I clone the repo. It looks like I should be able to switch directly to the c++-concepts branch. Or is there some configuration that has to happen on the remote site? How will this change the workflow? What's the process for submitting changes using git? Andrew On Thu, Jun 20, 2013 at 8:57 AM, Jason Merrill wrote: > On 06/20/2013 09:18 AM, Andrew Sutton wrote: >> >> I'll also create a github version of this branch, so can avoid email >> patches. > > > Why there rather than in the gcc.gnu.org git repository? > > http://gcc.gnu.org/wiki/GitMirror > > Jason > -- Andrew Sutton andrew.n.sut...@gmail.com
Re: [c++-concepts] code review
On 06/20/2013 09:18 AM, Andrew Sutton wrote: I'll also create a github version of this branch, so can avoid email patches. Why there rather than in the gcc.gnu.org git repository? http://gcc.gnu.org/wiki/GitMirror Jason
Re: [c++-concepts] code review
That works. I think the current patch addresses all of Jason's comments. I'll also create a github version of this branch, so can avoid email patches. On Thu, Jun 20, 2013 at 8:09 AM, Gabriel Dos Reis wrote: > Jason Merrill writes: > > | On 06/20/2013 01:30 AM, Gabriel Dos Reis wrote: > | > As I discussed > | > with Andrew a couple of weeks ago, I have been holding back the > | > merge from trunk because he has these patch series in the queue. > | > | Incidentally, since the code is going onto a branch, we don't really > | need to delay checkins based on code review; I'd actually rather > | review the code from within git than from an email patch. > > Makes sense. Andrew, you can check in what you have now, and refine > based on all other comments. I won't have network connection until > tonight; at that point I will do the merge from trunk. We don't want to > have the branch too old compared to the trunk. > > -- Gaby -- Andrew Sutton andrew.n.sut...@gmail.com
Re: [c++-concepts] code review
Jason Merrill writes: | On 06/20/2013 01:30 AM, Gabriel Dos Reis wrote: | > As I discussed | > with Andrew a couple of weeks ago, I have been holding back the | > merge from trunk because he has these patch series in the queue. | | Incidentally, since the code is going onto a branch, we don't really | need to delay checkins based on code review; I'd actually rather | review the code from within git than from an email patch. Makes sense. Andrew, you can check in what you have now, and refine based on all other comments. I won't have network connection until tonight; at that point I will do the merge from trunk. We don't want to have the branch too old compared to the trunk. -- Gaby
Re: [c++-concepts] code review
On 06/20/2013 01:30 AM, Gabriel Dos Reis wrote: As I discussed with Andrew a couple of weeks ago, I have been holding back the merge from trunk because he has these patch series in the queue. Incidentally, since the code is going onto a branch, we don't really need to delay checkins based on code review; I'd actually rather review the code from within git than from an email patch. Jason
Re: [c++-concepts] code review
On Wed, Jun 19, 2013 at 9:21 AM, Jason Merrill wrote: > On 06/18/2013 12:27 PM, Andrew Sutton wrote: >> >> There was a bug in instantiation_dependent_expr_r that would cause >> trait expressions like __is_class(int) to be marked as type dependent. >> It was always testing the 2nd operand, even for unary traits >> (NULL_TREE turns out to be type dependent). > > > I fixed that last month: > > 2013-05-20 Jason Merrill > > PR c++/57016 > * pt.c (instantiation_dependent_r) [TRAIT_EXPR]: Only check > type2 if there is one. The last merge to c++-concepts was a little bit before that (2013-06-16), so the fix wasn't on the branch. As I discussed with Andrew a couple of weeks ago, I have been holding back the merge from trunk because he has these patch series in the queue. That also means we don't get these sort of fixes before a while. Maybe I should just go ahead with the merge so that we have conflicts, and potentially less duplication of work in terms of fixing the compiler. -- Gaby
Re: [c++-concepts] code review
>> +// If the types of the underlying templates match, compare >> +// their constraints. The declarations could differ there. >> +if (types_match) >> + types_match = equivalent_constraints (get_constraints >> (olddecl), >> +current_template_reqs); > > > We can't assume that current_template_reqs will always apply to newdecl > here, as decls_match is called in overload resolution as well. What's the > problem with attaching the requirements to the declaration before we get to > duplicate_decls? It's because newdecl doesn't have a template_info at the point at which this is called, and the constraints are associated through that information. This seems like another good reason for keeping constraints with template decls. Until I change that, I can just test to see if newdecl has template info. If so, I'll use its constraints. If not, I'll use the current requirements. Andrew
Re: [c++-concepts] code review
On 06/18/2013 12:27 PM, Andrew Sutton wrote: There was a bug in instantiation_dependent_expr_r that would cause trait expressions like __is_class(int) to be marked as type dependent. It was always testing the 2nd operand, even for unary traits (NULL_TREE turns out to be type dependent). I fixed that last month: 2013-05-20 Jason Merrill PR c++/57016 * pt.c (instantiation_dependent_r) [TRAIT_EXPR]: Only check type2 if there is one. If you want to keep the is_binary_trait stuff, that's fine, except that +extern bool is_binary_trait (cp_trait_kind); ... +inline bool +is_binary_trait (cp_trait_kind k) violates the rules for inline functions: an inline function must be declared as inline before any uses and defined in all translation units that use it. +// reduced terms in the constraints language. Note that conjoining with a +// non-null expression with NULL_TREE is an identity operation. That is, Drop the first "with". +// If the types of the underlying templates match, compare +// their constraints. The declarations could differ there. +if (types_match) + types_match = equivalent_constraints (get_constraints (olddecl), +current_template_reqs); We can't assume that current_template_reqs will always apply to newdecl here, as decls_match is called in overload resolution as well. What's the problem with attaching the requirements to the declaration before we get to duplicate_decls? Jason
Re: [c++-concepts] code review
On Mon, Jun 17, 2013 at 2:20 PM, Jason Merrill wrote:
>> I have not thought deeply about constrained friend declarations. What
>> would a friend temploid look like?
>
>
> I was thinking something like
>
> template struct A {
> T t;
> requires Addable()
> friend A operator+(const A& a1, const A& a2)
> { return A(a1.t + a2.t); }
>
> };
We agreed about a month ago to have something like this
for member functions. It makes sense that it applies to friend
too (since it already applies to static member functions.)
One caveat in the design is that the nested requirement can
only add to the outer requirements.
-- Gaby
Re: [c++-concepts] code review
On 06/17/2013 02:10 PM, Andrew Sutton wrote:
You mean you don't need anymore in logic.cc? I think we want
the include in general if we're going to support people using the
C++ standard library.
I don't. Those decisions are above my pay grade, so I'm doing my best
not to make them.
If you don't need the change for concepts any more, feel free to drop it.
Can friend temploids be constrained?
I have not thought deeply about constrained friend declarations. What
would a friend temploid look like?
I was thinking something like
template struct A {
T t;
requires Addable()
friend A operator+(const A& a1, const A& a2)
{ return A(a1.t + a2.t); }
};
I'm not clear on the issue. Perhaps leaving processing_template_decl alone
and using fold_non_dependent_expr would accomplish what you want?
I don't think that will work. The problem comes from the instantiation
of traits (and some other expressions) during constraint checking.
When processing_template_decl is non-zero, finish_trait_expr will
create TRAIT_EXPR nodes, which aren't handled by the constexpr
evaluation engine.
Sure, but fold_non_dependent_expr should turn the TRAIT_EXPR into a more
useful form.
Can explicit specializations have constraints, to indicate which template
they are specializing?
Good question. I don't think so. I believe that it would be a
specialization of the most specialized function template whose
constraints were satisfied. So:
Makes sense.
Passing 'true' for require_all_args means no deduction will be done; rather,
all arguments must either be specified or have default arguments.
I see. It seems like I should be passing false here, since I want to
ensure that the resulting argument list can be used to instantiate the
template.
I think true is what you want, since there are no function arguments to
do argument deduction from. Passing true for require_all_args
guarantees that the result can be used to instantiate the template;
passing false can return an incomplete set of arguments that will be
filled in later by fn_type_unification.
Jason
Re: [c++-concepts] code review
>> 2. I left the include in system.h, because removing it will
>> result in errors due to an inclusion of . It's probable
>> that both can be removed, but I didn't test it with this patch.
>
>
> You mean you don't need anymore in logic.cc? I think we want
> the include in general if we're going to support people using the
> C++ standard library.
I don't. Those decisions are above my pay grade, so I'm doing my best
not to make them.
>> + reason = template_unification_error_rejection ();
>
>
> Can't you do
>
> template_constraint_failure (DECL_TI_TEMPLATE (fn), DECL_TI_ARGS (fn))
I can indeed. Fixed.
>>if (fn == error_mark_node)
>> {
>> + if (!check_template_constraints (tmpl, targs))
>> + reason = template_constraint_failure (tmpl, targs);
>> + else if (errorcount+sorrycount == errs)
>>/* Don't repeat unification later if it already resulted in errors.
>> */
>
>
> A constraint failure is a form of unification failure, like SFINAE; let's
> handle it in that context rather than separately here, and reserve
> rr_constraint_failure for the case of a non-template member function.
And emit the diagnostics in fn_type_unification when explain_p is set?
Seems reasonable.
> The uses of "actual template" in the comment and function name seem to mean
> different things. Maybe call the function template_decl_for_candidate?
Fixed.
> Can friend temploids be constrained?
I have not thought deeply about constrained friend declarations. What
would a friend temploid look like?
> Seems like the comment is no longer accurate; the function now only returns
> NULL_TREE if both a and b are NULL_TREE.
It's not. And I removed disjoin_requirements. I don't think it will
ever be used.
>> +static tree
>> +get_constraint_check (tree call)
>
> The comment needs updating. And the function isn't used anywhere; it seems
> redundant with resolve_constraint_check.
I removed the function.
>> +// and other template nodes from generating new expressions
>> +// during instantiation.
>
> I'm not clear on the issue. Perhaps leaving processing_template_decl alone
> and using fold_non_dependent_expr would accomplish what you want?
I don't think that will work. The problem comes from the instantiation
of traits (and some other expressions) during constraint checking.
When processing_template_decl is non-zero, finish_trait_expr will
create TRAIT_EXPR nodes, which aren't handled by the constexpr
evaluation engine.
I could updated constexpr to fix that, or changed finish_trait_expr
(and others? noexcept maybe) to evaluate for non-dependent arguments,
but I was trying to limit the scope of my changes.
I'd really like for this structure to go away. It's very fragile.
>> +// Check the template's constraints for the specified arguments,
>> +// returning true if the constraints are satisfied and false
>> +// otherwise.
>> +bool
>> +check_template_constraints (tree t, tree args)
>> +{
>> + return check_constraints (get_constraints (t), args);
>> +}
>
>
> Seems like the last function would also work for types and non-template
> decls.
It should. Updated the comment to reflect the broader applicability of
the function.
>> + error_at (loc, "constraints not satisfied");
>
>
> I assume you're planning to make this more verbose? It could use a TODO to
> that effect.
Very much so. Added comments.
>> + else if (are_constrained_overloads (newdecl, olddecl))
>> +{
>> + // If newdecl and olddecl are function templates with the
>> + // same type but different constraints, then they cannot be
>> + // duplicate declarations. However, if the olddecl is declared
>> + // as a concept, then the program is ill-formed.
>> + if (DECL_DECLARED_CONCEPT_P (DECL_TEMPLATE_RESULT (olddecl)))
>> +{
>> + error ("cannot specialize concept %q#D", olddecl);
>> + return error_mark_node;
>> +}
>> + return NULL;
>> +}
>
>
> We should check for differing constraints in decls_match, and then this code
> should be in the !types_match section of duplicate_decls.
I'll get back to you on this. I need to work my way through that code.
>
>>error ("concept %q#D result must be convertible to bool", fn);
>
>
> Why don't we require the result to actually be bool? It seems difficult to
> decompose dependent requirements if the return type can be something else.
I'm probably sitting somewhere between two ideas. Requiring the result
to be exactly bool is fine.
>> + cp_lexer_next_token_is_keyword (parser->lexer,
>> RID_REQUIRES))
>>{
>> tree reqs = release (current_template_reqs);
>> -if (tree r = cp_parser_requires_clause_opt (parser))
>> +if (tree r = cp_parser_requires_clause (parser))
>
>
> I was thinking to change the name of cp_requires_clause to
> cp_parser_requires_clause_opt and change the assert to a test for the
> keyword and return NULL_TREE if it isn't found.
I s
Re: [c++-concepts] code review
On Fri, Jun 14, 2013 at 8:40 PM, Jason Merrill wrote: >> +// \Gamma, P |- Delta\Gamma, Q |- \Delta > > > Are the \ for TeX markup or something? You're missing one on the left Delta > here. yes, I think the backslash was for LaTeX, but we get warnings about having backslash in comments, so I think we can safely leave them out. -- Gaby
Re: [c++-concepts] code review
1. tree_template_info still contains constraint_info. That will change
in a subsequent patch. I'm kind of waiting for specializations to get
TEMPLATE_DECLs.
Makes sense.
2. I left the include in system.h, because removing it will
result in errors due to an inclusion of . It's probable
that both can be removed, but I didn't test it with this patch.
You mean you don't need anymore in logic.cc? I think we
want the include in general if we're going to support people
using the C++ standard library.
+ if (DECL_USE_TEMPLATE (fn))
+{
+ tree cons = DECL_TEMPLATE_CONSTRAINT (fn);
+ if (!check_constraints (cons))
+{
+ // TODO: Fail for the right reason.
+ reason = template_unification_error_rejection ();
Can't you do
template_constraint_failure (DECL_TI_TEMPLATE (fn), DECL_TI_ARGS (fn))
?
if (fn == error_mark_node)
{
+ if (!check_template_constraints (tmpl, targs))
+ reason = template_constraint_failure (tmpl, targs);
+ else if (errorcount+sorrycount == errs)
/* Don't repeat unification later if it already resulted in errors. */
A constraint failure is a form of unification failure, like SFINAE;
let's handle it in that context rather than separately here, and reserve
rr_constraint_failure for the case of a non-template member function.
+// Returns the template declaration associated with the candidate
+// function. For actual templates, this is directly associated
+// with the candidate. For temploids, we return the template
+// associated with the specialization.
+static inline tree
+get_actual_template (struct z_candidate *cand)
The uses of "actual template" in the comment and function name seem to
mean different things. Maybe call the function template_decl_for_candidate?
+ // Non-temploids cannot be constrained.
Can friend temploids be constrained?
+ if (!DECL_TEMPLOID_INSTANTIATION (new_fn) &&
+ !DECL_TEMPLOID_INSTANTIATION (old_fn))
The && goes at the beginning of the second line.
+// reduced terms in the constraints language. Returns NULL_TREE if either A or
// B are NULL_TREE.
tree
conjoin_requirements (tree a, tree b)
{
- if (a && b)
-return build_min (TRUTH_ANDIF_EXPR, boolean_type_node, a, b);
+ if (a)
+return b ? join_requirements (TRUTH_ANDIF_EXPR, a, b) : a;
+ else if (b)
+return b;
else
return NULL_TREE;
Seems like the comment is no longer accurate; the function now only
returns NULL_TREE if both a and b are NULL_TREE.
+// Returns true if the function decl F is a constraint predicate. It must be a
+// constexpr, nullary function with a boolean result type.
+static tree
+get_constraint_check (tree call)
The comment needs updating. And the function isn't used anywhere; it
seems redundant with resolve_constraint_check.
+// The raison d'entre of this class is to prevent traits
"d'etre"
+// and other template nodes from generating new expressions
+// during instantiation.
I'm not clear on the issue. Perhaps leaving processing_template_decl
alone and using fold_non_dependent_expr would accomplish what you want?
+static inline bool
+check_type_constraints (tree t, tree args)
+{
+ return check_constraints (CLASSTYPE_TEMPLATE_CONSTRAINT (t), args);
+}
+
+static inline bool
+check_decl_constraints (tree t, tree args)
+{
+ if (TREE_CODE (t) == TEMPLATE_DECL)
+return check_decl_constraints (DECL_TEMPLATE_RESULT (t), args);
+ else
+return check_constraints (DECL_TEMPLATE_CONSTRAINT (t), args);
+}
+
+// Check the template's constraints for the specified arguments,
+// returning true if the constraints are satisfied and false
+// otherwise.
+bool
+check_template_constraints (tree t, tree args)
+{
+ return check_constraints (get_constraints (t), args);
+}
Seems like the last function would also work for types and non-template
decls.
+ error_at (loc, "constraints not satisfied");
I assume you're planning to make this more verbose? It could use a TODO
to that effect.
+// and template isntantiation. Evaluation warnings are also inhibited.
"instantiation"
+ else if (are_constrained_overloads (newdecl, olddecl))
+{
+ // If newdecl and olddecl are function templates with the
+ // same type but different constraints, then they cannot be
+ // duplicate declarations. However, if the olddecl is declared
+ // as a concept, then the program is ill-formed.
+ if (DECL_DECLARED_CONCEPT_P (DECL_TEMPLATE_RESULT (olddecl)))
+{
+ error ("cannot specialize concept %q#D", olddecl);
+ return error_mark_node;
+}
+ return NULL;
+}
We should check for differing constraints in decls_match, and then this
code should be in the !types_match section of duplicate_decls.
error ("concept %q#D result must be convertible to bool", fn);
Why don't we require the result to actually be bool? It seems difficult
to decompose dependent requirements if the return ty
Re: [c++-concepts] code review
On 06/12/2013 11:53 AM, Gabriel Dos Reis wrote: I am still surprised though that we don't generate TEMPLATE_DECLs for partial instantiations (since they are still morally templates.) Yes, we should. Jason
Re: [c++-concepts] code review
On Mon, Jun 10, 2013 at 2:30 PM, Jason Merrill wrote: > On 06/08/2013 09:34 AM, Andrew Sutton wrote: >> >> I think I previously put constraint_info in lang_decl_min, right next >> to template_info no less. It was easy to manage there, and initialized >> as part of build_template_decl. But this obviously doesn't work for >> partial specializations unless they get template_decls. > > > Right. And we would want it to be specific to template_decls, not all decls > that use lang_decl_min. yes, exactly my feedback on the original implementation. I am still surprised though that we don't generate TEMPLATE_DECLs for partial instantiations (since they are still morally templates.) -- Gaby
Re: [c++-concepts] code review
On 06/11/2013 11:09 AM, Andrew Sutton wrote: And you need to do this even when the call is type-dependent? Yes. Especially when the call is type-dependent. That's how I generate the constraint sets, which are used to determine the most constrained template during overload resolution. Ah, that makes sense. Then I guess what you're doing in resolve_constraint_check is what you want; since the function takes no parameters and returns bool, there's no substitution to do into the function type, so SFINAE doesn't matter. I'd just ask again to rename substitute_template_parameters; perhaps just calling it coerce_template_parms makes sense. And also please pass tmpl down to the is_decl parameter. Jason
Re: [c++-concepts] code review
> And you need to do this even when the call is type-dependent? Yes. Especially when the call is type-dependent. That's how I generate the constraint sets, which are used to determine the most constrained template during overload resolution.
Re: [c++-concepts] code review
On 06/11/2013 10:49 AM, Andrew Sutton wrote: After investigating, neither call_expr nor resolve_nondeduced_context do what I need. I need a resolution of a call expression that does not return overload sets, especially in the case where the initial call expression is already dependent. Does this have to do with restrictions on overloading of concept functions? Very much so. I need a single function because I'm inlining its body at the call site. And you need to do this even when the call is type-dependent? Jason
Re: [c++-concepts] code review
>> After investigating, neither call_expr nor resolve_nondeduced_context >> do what I need. I need a resolution of a call expression that does not >> return overload sets, especially in the case where the initial call >> expression is already dependent. > > > Does this have to do with restrictions on overloading of concept functions? Very much so. I need a single function because I'm inlining its body at the call site. Andrew
Re: [c++-concepts] code review
On 06/11/2013 09:45 AM, Andrew Sutton wrote: After investigating, neither call_expr nor resolve_nondeduced_context do what I need. I need a resolution of a call expression that does not return overload sets, especially in the case where the initial call expression is already dependent. Does this have to do with restrictions on overloading of concept functions? Jason
Re: [c++-concepts] code review
>> I think I previously put constraint_info in lang_decl_min, right next >> to template_info no less. It was easy to manage there, and initialized >> as part of build_template_decl. But this obviously doesn't work for >> partial specializations unless they get template_decls. > > > Right. And we would want it to be specific to template_decls, not all decls > that use lang_decl_min. I'll have to check. I can't remember off the top of my head if non-template member functions have a corresponding template_decl. I think they do. auto declarations will also get constraints in the future. >> On the topic of logic.cc, I'm plan on rewriting this module to use a >> set instead of lists. There will be some pretty substantial changes to >> the internal interfaces. >> >> Would it be reasonable to commit this now (since it works correctly), >> and plan to rewrite it in the near future? > > > OK. I was experimenting with this over the weekend. I'm just going to rewrite it now, but without the optimizations I alluded to earlier. They didn't pan out the way I'd have liked. >> I think I see where the problem is. cp_parser_requires_clause is >> parsed non-optionally in a requires expression, but that's not >> included in the patch. I factored out the explicit parsing (hence the >> assertion) from the optional parsing. > > > The two situations seem pretty similar; you don't decide you're parsing a > requires expression until you see the keyword either, right? > > The usual pattern in the parser is for a function to try to parse a > particular construct and then return NULL_TREE if we're looking at something > else; it seems most straightforward to do that in this case as well. Yes, but I wasn't testing for the keyword prior to the call to cp_parser_requires_clause_opt. That's not quite true, I was in for member functions, but that was an oversight. Changing to test for requires won't be hard. >> What is the main entry point to overload resolution? > > > Perhaps finish_call_expr is what you want. After investigating, neither call_expr nor resolve_nondeduced_context do what I need. I need a resolution of a call expression that does not return overload sets, especially in the case where the initial call expression is already dependent. resolve_nondeduced_context looks like a good candidate to extend, but I hesitate to modify since it's used in a number of different places, and winnowing the overload set may not be appropriate in those contexts.
Re: [c++-concepts] code review
On 6/10/13, Diego Novillo wrote: > On 2013-06-09 20:34 , Gabriel Dos Reis wrote: > > So, my advice is for GCC source code to forget about the > > headers for the most part. I can see an instance where > > or would make a difference but given point (1) above, > > no it doesn't. Just use the traditional headers and > > be done with it. > > > > Maybe I should have included this in our C++ coding standards, > > but I don't know how Benjamin, Lawrence, and Diego fee about it. > > Sounds reasonable to me. Me too. The split in headers always felt excessively artifical to me. -- Lawrence Crowl
Re: [c++-concepts] code review
On 06/08/2013 09:34 AM, Andrew Sutton wrote: I think I previously put constraint_info in lang_decl_min, right next to template_info no less. It was easy to manage there, and initialized as part of build_template_decl. But this obviously doesn't work for partial specializations unless they get template_decls. Right. And we would want it to be specific to template_decls, not all decls that use lang_decl_min. I'd be okay committing the current design with the caveat that it may need to be rewritten in the not-so-distant future. I've already written the other other way two or three times, so I'm familiar with those changes. Yeah, that sounds fine. On the topic of logic.cc, I'm plan on rewriting this module to use a set instead of lists. There will be some pretty substantial changes to the internal interfaces. Would it be reasonable to commit this now (since it works correctly), and plan to rewrite it in the near future? OK. I think I see where the problem is. cp_parser_requires_clause is parsed non-optionally in a requires expression, but that's not included in the patch. I factored out the explicit parsing (hence the assertion) from the optional parsing. The two situations seem pretty similar; you don't decide you're parsing a requires expression until you see the keyword either, right? The usual pattern in the parser is for a function to try to parse a particular construct and then return NULL_TREE if we're looking at something else; it seems most straightforward to do that in this case as well. What is the main entry point to overload resolution? Perhaps finish_call_expr is what you want. Jason
Re: [c++-concepts] code review
On 06/09/2013 08:49 PM, Gabriel Dos Reis wrote: If you put the function in an unnamed namespace you would expect GCC to treat is as if it was of internal linkage for many purposes including automatic inlining, but it doesn't:-( For example, you lose the "defined but not used warning", and the "used but not defined" warnings :-(( Indeed, G++ currently only gives those warnings for things declared 'static', but that's trivial to fix, and shouldn't affect other things in the compiler. Jason
Re: [c++-concepts] code review
On 06/09/2013 08:34 PM, Gabriel Dos Reis wrote: I strongly suggest prefering over for GCC source code base. The problem is that including does not define _GLIBCXX_CSTDLIB, so if one of the C++ library headers includes the contents are added then, but by that point e.g. "malloc" is poisoned, so the mentions of malloc in cstdlib cause errors. Because we are poisoning these names, we need to #include *both* headers in system.h. Jason
Re: [c++-concepts] code review
On 2013-06-09 20:34 , Gabriel Dos Reis wrote: So, my advice is for GCC source code to forget about the headers for the most part. I can see an instance where or would make a difference but given point (1) above, no it doesn't. Just use the traditional headers and be done with it. Maybe I should have included this in our C++ coding standards, but I don't know how Benjamin, Lawrence, and Diego fee about it. Sounds reasonable to me. Diego.
Re: [c++-concepts] code review
On Sat, Jun 8, 2013 at 8:34 AM, Andrew Sutton wrote: >> template >> tree >> extract_goals (proof_state& s) >> ... >> return extract_goals(s); >> >> but I suppose STL style is OK, too. > > > Huh. I didn't realize that could be inlined. Neat. We do -- we have been doing so for quite some time now, including devirtualization and inlining of functions with internal linkage. The reason why I tend to prefer the function argument style over template argument-style is where I don't want to specify everything, but rather prefer type deduction. Which in my case is almost aways. There is one thing though: C++03 does not allow using a function with internal linkage or no linkage as template arguments. If you put the function in an unnamed namespace you would expect GCC to treat is as if it was of internal linkage for many purposes including automatic inlining, but it doesn't :-( For example, you lose the "defined but not used warning", and the "used but not defined" warnings :-(( -- Gaby
Re: [c++-concepts] code review
On Sun, Jun 9, 2013 at 3:34 PM, Oleg Endo wrote: > On Thu, 2013-06-06 at 16:29 -0400, Jason Merrill wrote: >> On 06/06/2013 01:47 PM, Andrew Sutton wrote: >> > I never did understand why this happens. Compiling with GCC-4.6, I get >> > these errors originating in logic.cc from an include of . >> > This is what I get: >> > >> > /usr/include/c++/4.6/cstdlib:76:8: error: attempt to use poisoned "calloc" >> >> Ah, I see: adding the include gets the mentions of malloc in before the >> names are poisoned. This change is OK. >> > > I ran into the same issue when I started using C++ std:: stuff in the SH > backend code last year. I posted a patch, but somehow it didn't go > anywhere... > > http://gcc.gnu.org/ml/gcc-patches/2012-09/msg00880.html > > The workaround was to include as the first include in sh.c. > Would it be possible to have the change above also in trunk? > > Cheers, > Oleg > I strongly suggest prefering over for GCC source code base. The reason is that it brings us very little: 1. Not all compilers implement C++93/C++03 semantics on all platforms; in fact even GCC didn't on solaris platforms for example. So, from bootstrapping purposes, we better be on the safer side. 2. C++11 says that the implementation is free to define names in both namespaces (global and std.) If we ever accept C++11 in 5-10 years, we better have something can withstand the evolution. So, my advice is for GCC source code to forget about the headers for the most part. I can see an instance where or would make a difference but given point (1) above, no it doesn't. Just use the traditional headers and be done with it. Maybe I should have included this in our C++ coding standards, but I don't know how Benjamin, Lawrence, and Diego fee about it. -- Gaby
Re: [c++-concepts] code review
On Thu, 2013-06-06 at 16:29 -0400, Jason Merrill wrote: > On 06/06/2013 01:47 PM, Andrew Sutton wrote: > > I never did understand why this happens. Compiling with GCC-4.6, I get > > these errors originating in logic.cc from an include of . > > This is what I get: > > > > /usr/include/c++/4.6/cstdlib:76:8: error: attempt to use poisoned "calloc" > > Ah, I see: adding the include gets the mentions of malloc in before the > names are poisoned. This change is OK. > I ran into the same issue when I started using C++ std:: stuff in the SH backend code last year. I posted a patch, but somehow it didn't go anywhere... http://gcc.gnu.org/ml/gcc-patches/2012-09/msg00880.html The workaround was to include as the first include in sh.c. Would it be possible to have the change above also in trunk? Cheers, Oleg
Re: [c++-concepts] code review
>> +C++ ObjC++ Var(flag_concepts, true) > > This line declares flag_concepts implicitly. Ah... I see. Fixed. >> That's the long and short of it. Gaby suggested writing constraints >> here so that, for any instantiation, you would have easy access to the >> constraints for that declaration. > > I'm not sure why you would care about the constraints for a particular > instantiation; constraints only apply to the template, right? In concepts lite, yes. Moving forward... I'm less certain. In the context of separate checking, instantiated requirements may carry lookup information into the current instantiation. But that's just speculation. I think I previously put constraint_info in lang_decl_min, right next to template_info no less. It was easy to manage there, and initialized as part of build_template_decl. But this obviously doesn't work for partial specializations unless they get template_decls. I'd be okay committing the current design with the caveat that it may need to be rewritten in the not-so-distant future. I've already written the other other way two or three times, so I'm familiar with those changes. >> branch_goal queues a copy of the current sub-goal, returning a >> reference to that new branch. The insertion of the operands are done >> on different sub-goals, giving the expected results. > > Right, I suppose I should have paid more attention to "This does not update > the current goal"... On the topic of logic.cc, I'm plan on rewriting this module to use a set instead of lists. There will be some pretty substantial changes to the internal interfaces. Would it be reasonable to commit this now (since it works correctly), and plan to rewrite it in the near future? > template > tree > extract_goals (proof_state& s) > ... > return extract_goals(s); > > but I suppose STL style is OK, too. Huh. I didn't realize that could be inlined. Neat. >> I was trying to write the parsing code a little more modularly so I >> could keep my parse functions as small as possible. I use the facility >> more heavily in the requires/validexpr code that's not included here. > > > Hmm, to me it seems more modular to keep all of the code for handling e.g. > "requires" in its own function rather than needing two different places to > know how a requires clause starts. I think I see where the problem is. cp_parser_requires_clause is parsed non-optionally in a requires expression, but that's not included in the patch. I factored out the explicit parsing (hence the assertion) from the optional parsing. I could remove the assertion. There's no path to that function that does not first check that 'requires' has been found. >>> Why don't you use 'release' and conjoin_requirements here? >> >> >> Because there is no template parameter list that can provide >> additional requirements in this declaration. > > > OK, please add a comment to that effect. On second thought, the absence of release() may actually have been a bug. Comment added. >>> The comment sounds more like tsubst_template_parms than >>> coerce_template_parms. >> >> >> It might be... I'll have to look. What I actually want to get is the >> set of actual arguments that will be substituted for template >> parameters given an initial set of arguments (lookup default >> arguments, generate pack arguments, etc). > > > Right, I think coerce_template_parms has the effect you want, I just don't > think of it as doing substitution, so the comment and name could use a > tweak. If the function doesn't go away, that is. Still looking at this. What is the main entry point to overload resolution? Andrew
Re: [c++-concepts] code review
On 06/06/2013 01:47 PM, Andrew Sutton wrote: I never did understand why this happens. Compiling with GCC-4.6, I get these errors originating in logic.cc from an include of . This is what I get: /usr/include/c++/4.6/cstdlib:76:8: error: attempt to use poisoned "calloc" Ah, I see: adding the include gets the mentions of malloc in before the names are poisoned. This change is OK. +; Activate C++ concepts support. +Variable +bool flag_concepts You don't need to declare this separately. I'm not quite sure what you mean. Separately from what? Separately from +C++ ObjC++ Var(flag_concepts, true) This line declares flag_concepts implicitly. That's the long and short of it. Gaby suggested writing constraints here so that, for any instantiation, you would have easy access to the constraints for that declaration. I'm not sure why you would care about the constraints for a particular instantiation; constraints only apply to the template, right? branch_goal queues a copy of the current sub-goal, returning a reference to that new branch. The insertion of the operands are done on different sub-goals, giving the expected results. Right, I suppose I should have paid more attention to "This does not update the current goal"... +template + tree + extract_goals (proof_state& s, F proj) Why is proj a function argument rather than a template argument, which would allow inlining? STL influence. Can you give an example of how this should look in order to take advantage of inlining? I was thinking something like template tree extract_goals (proof_state& s) ... return extract_goals(s); but I suppose STL style is OK, too. It was used in a previous version, and I suspect it might be useful in the future, but I'm not 100% sure. I felt it would be worthwhile to keep it in the patch just in case. Makes sense. And why do it this way rather than check and possibly return at the top of the function, as elsewhere in the parser? You already have cp_parser_requires_clause checking for RID_REQUIRES. I was trying to write the parsing code a little more modularly so I could keep my parse functions as small as possible. I use the facility more heavily in the requires/validexpr code that's not included here. Hmm, to me it seems more modular to keep all of the code for handling e.g. "requires" in its own function rather than needing two different places to know how a requires clause starts. Why don't you use 'release' and conjoin_requirements here? Because there is no template parameter list that can provide additional requirements in this declaration. OK, please add a comment to that effect. +// Try to substitute ARGS into PARMS, returning the actual list of +// arguments that have been substituted. If ARGS cannot be substituted, +// return error_mark_node. The comment sounds more like tsubst_template_parms than coerce_template_parms. It might be... I'll have to look. What I actually want to get is the set of actual arguments that will be substituted for template parameters given an initial set of arguments (lookup default arguments, generate pack arguments, etc). Right, I think coerce_template_parms has the effect you want, I just don't think of it as doing substitution, so the comment and name could use a tweak. If the function doesn't go away, that is. Jason
Re: [c++-concepts] code review
Hi Jason,
Thanks for the comments. I just went ahead and fixed all the editorial
issues. Comments and questions below:
>> * gcc/system.h (cstdlib): Include to avoid poisoned
>> declaration errors.
>
> Poisoned declarations of what? This seems redundant with the #include
> just below.
I never did understand why this happens. Compiling with GCC-4.6, I get
these errors originating in logic.cc from an include of .
This is what I get:
In file included from /usr/include/c++/4.6/bits/stl_algo.h:61:0,
from /usr/include/c++/4.6/algorithm:63,
from ../../c++-concepts/gcc/cp/logic.cc:45:
/usr/include/c++/4.6/cstdlib:76:8: error: attempt to use poisoned "calloc"
/usr/include/c++/4.6/cstdlib:83:8: error: attempt to use poisoned "malloc"
/usr/include/c++/4.6/cstdlib:89:8: error: attempt to use poisoned "realloc"
/usr/include/c++/4.6/cstdlib:112:11: error: attempt to use poisoned "calloc"
/usr/include/c++/4.6/cstdlib:119:11: error: attempt to use poisoned "malloc"
/usr/include/c++/4.6/cstdlib:127:11: error: attempt to use poisoned "realloc"
>> + /* Concepts-related keywords */
>> + { "assume", RID_ASSUME, D_CXXONLY | D_CXX0X | D_CXXWARN },
>> + { "axiom", RID_AXIOM, D_CXXONLY | D_CXX0X | D_CXXWARN },
>> + { "concept", RID_CONCEPT,D_CXXONLY | D_CXX0X | D_CXXWARN },
>> + { "forall", RID_FORALL, D_CXXONLY | D_CXX0X | D_CXXWARN },
>> + { "requires",RID_REQUIRES, D_CXXONLY | D_CXX0X | D_CXXWARN },
>
>
> I don't see anything that limits these keywords to when concepts are
> enabled. You probably want to add an additional mask that applies to these.
Ok. I'll add D_CXX_CONCEPTS and set it for all of reserved words.
>> +; Activate C++ concepts support.
>> +Variable
>> +bool flag_concepts
>
> You don't need to declare this separately.
I'm not quite sure what you mean. Separately from what?
>> +static tree
>> +resolve_constraint_check (tree ovl, tree args)
>
> This function seems to be trying to model a subset of overload resolution,
> which seems fragile to me; better to use the actual overload resolution code
> to decide which function the constraint expression calls, or at least
> resolve_nondeduced_context which handles SFINAE.
It is. I was a little hesitant to use the actual overload resolution
facility because of the restrictions for concepts. I think I was also
doing something a little different in previous version.
I'll take another look and see if either will work instead of my
homebrew solution.
>
>> +case CAST_EXPR:
>> + return reduce_node (TREE_VALUE (TREE_OPERAND (t, 0)));
>
> Are we assuming that constraint expressions can't involve objects of literal
> class type?
For now, I think it's a reasonable restriction. We can relax this as
needed in the future.
>> struct GTY(()) tree_template_info {
>>struct tree_common common;
>> + tree constraint;
>>vec
>> *typedefs_needing_access_checking;
>> };
>
>
> Why do we need constraint information in template_info? I suppose this is
> the issue you raised in your mail last month:
>
>> I had expected there to be a template decl associated with underlying
>> class, but after print-bombing most of the instantiation, lookup, and
>> specialization processing routines, I couldn't find that one was ever
>> created for the type decl.
>
> This does seem like a shortcoming, that also led to the typedefs vec getting
> stuck into the template_info inappropriately. I guess we should start
> building TEMPLATE_DECLs for partial specializations.
That's the long and short of it. Gaby suggested writing constraints
here so that, for any instantiation, you would have easy access to the
constraints for that declaration.
>> +struct GTY(()) tree_constraint_info {
>> + struct tree_base base;
>> + tree spelling;
>> + tree requirements;
>> + tree assumptions;
>> +};
>
>
> I'm confused by the relationship between the comment and the field names
> here. Where do the conclusions come in? Is "requirements (as a constant
> expression)" in the spelling or requirements field?
I must have forgotten to update the comments. I probably need to
re-think this structure a bit. The requirements field is the complete
set of requirement (shorthand constraints + requires clause). The
assumptions field is the analyzed requirements.
I was using the "spelling" field specifically for diagnostics, so I
could print exactly what was written. I think it might be better to
hang that off the template parameter list rather than the constraint
info.
I don't think "spelling" is used in the current patch other than initialization.
>> + DECL_DECLARED_CONCEPT_P (decl) = true;
>> + if (!check_concept_fn (decl))
>> +return NULL_TREE;
>> +}
>
>
> I think I'd rather deal with an invalid concept by not marking it as a
> concept, but still declaring it as a constexpr function.
Sounds reasonable.
>> +// Return the current list of assumed terms.
>>
[c++-concepts] code review
Hi, I'm finally going through the current code on the branch, sorry for
the delay.
* gcc/system.h (cstdlib): Include to avoid poisoned
declaration errors.
Poisoned declarations of what? This seems redundant with the #include
just below.
+ /* Concepts-related keywords */
+ { "assume", RID_ASSUME, D_CXXONLY | D_CXX0X | D_CXXWARN },
+ { "axiom", RID_AXIOM, D_CXXONLY | D_CXX0X | D_CXXWARN },
+ { "concept", RID_CONCEPT,D_CXXONLY | D_CXX0X | D_CXXWARN },
+ { "forall", RID_FORALL, D_CXXONLY | D_CXX0X | D_CXXWARN },
+ { "requires",RID_REQUIRES, D_CXXONLY | D_CXX0X | D_CXXWARN },
I don't see anything that limits these keywords to when concepts are
enabled. You probably want to add an additional mask that applies to these.
+; Activate C++ concepts support.
+Variable
+bool flag_concepts
You don't need to declare this separately.
Components for process constraints and evaluating constraints.
Should that be "processing"?
+// TODO: Simply assinging boolean_type_node to the result type of the
expression
"assigning"
+// reduced terms in the constraints languaage. Returns NULL_TREE if either A or
"language"
+// a constexpr, nullary function teplate whose result can be converted
"template"
+ // A constraint is declared constexpr
Needs a period.
+// This function is not called for abitrary call expressions. In particul,
"particular"
+static tree
+resolve_constraint_check (tree ovl, tree args)
This function seems to be trying to model a subset of overload
resolution, which seems fragile to me; better to use the actual overload
resolution code to decide which function the constraint expression
calls, or at least resolve_nondeduced_context which handles SFINAE.
+case CAST_EXPR:
+ return reduce_node (TREE_VALUE (TREE_OPERAND (t, 0)));
Are we assuming that constraint expressions can't involve objects of
literal class type?
+// If T is a call to a constraint instantiate it's definition and
"its"
+ tree c = finish_call_expr (t, &args, true, false, 0);
+ error ("invalid requirement");
+ inform (input_location, "did you mean %qE", c);
For both of these diagnostics, let's use EXPR_LOC_OR_HERE (t) as the
location.
+// Reduce the requirement T into a logical formula written in terms of
+// atomic propositions.
+tree
+reduce_requirements (tree reqs)
s/T/REQS/
struct GTY(()) tree_template_info {
struct tree_common common;
+ tree constraint;
vec *typedefs_needing_access_checking;
};
Why do we need constraint information in template_info? I suppose this
is the issue you raised in your mail last month:
In general constraints are directly associated with a template decl. For
example:
template
class complex;
The Arithmetic constraint is associated with the template decl. However, this
doesn't seem to work with partial specializations:
template
struct complex { ... };
I had expected there to be a template decl associated with underlying class,
but after print-bombing most of the instantiation, lookup, and specialization
processing routines, I couldn't find that one was ever created for the type
decl.
This does seem like a shortcoming, that also led to the typedefs vec
getting stuck into the template_info inappropriately. I guess we should
start building TEMPLATE_DECLs for partial specializations.
+/* Constraint information for a C++ declaration. This includes the
+ requirements (as a constant expression) and the decomposed assumptions
+ and conclusions. The assumptions and conclusions are cached for the
+ purposes of overlaod resolution and diagnostics. */
+struct GTY(()) tree_constraint_info {
+ struct tree_base base;
+ tree spelling;
+ tree requirements;
+ tree assumptions;
+};
I'm confused by the relationship between the comment and the field names
here. Where do the conclusions come in? Is "requirements (as a
constant expression)" in the spelling or requirements field?
Also, "overload".
+constraint_info_p (tree t)
+template_info_p (tree t)
Let's use check_* rather than *_p for these, too.
+// NODE must be a lang-decl.
Let's say "NODE must have DECL_LANG_SPECIFIC" to avoid confusion with
struct lang_decl.
+ error ("concept %q#D declared with function arguments", fn);
s/arguments/parameters/. Some of the gcc internals get this distinction
wrong; but we don't need to expose that in diagnostics...
+ // If the concept declaration specifier was found, check
+ // that the declaration satisfies the necessary requirements.
+ if (inlinep & 4)
+{
+ DECL_DECLARED_CONCEPT_P (decl) = true;
+ if (!check_concept_fn (decl))
+return NULL_TREE;
+}
I think I'd rather deal with an invalid concept by not marking it as a
concept, but still declaring it as a constexpr function.
+ flag_concepts = true;
This is redundant since c.opt specifies that it defaults to true.
+// Retur
