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 <cstdlib> to avoid poisoned
>> declaration errors.
>
> Poisoned declarations of what? This seems redundant with the #include
> <stdlib.h> 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 <algorithm>.
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<qualified_typedef_usage_t, va_gc>
>> *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.
>> +inline term_list &
>> +assumptions (proof_state &s) { return assumptions (current_goal (s)); }
>
>
> Just as a comment, this use of internal iterators and forwarding functions
> seems confusing; a reader is likely to think that assumptions (s) would
> produce a list of assumptions for the state as a whole, rather than for the
> current goal. It's particularly surprising in functions like
> decompose_left_goal, where we look at the innermost iterator and then pass
> the outermost object to a subroutine.
I'll think about how to restructure this to make it more intuitive.
>> +// And right logical rule.
>> +inline void
>> +and_right (proof_state &s)
>> +{
>> + gcc_assert (TREE_CODE (conclusion (s)) == TRUTH_ANDIF_EXPR);
>> + tree t = ignore_term (s);
>> + conclude_term (branch_goal (s), TREE_OPERAND (t, 1));
>> + conclude_term (current_goal (s), TREE_OPERAND (t, 0));
>> +}
>
> A |- B
> A |- C
>
> right? But since branch_goal updates the current sub-goal, I would expect
> the result to be
>
> A |-
> A |- C, B
>
> instead. And likewise for or_left.
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.
I have a long-term plan to rewrite these sub-goals as sets rather than
lists to improve performance. That would also let me worry less about
maintaining state, like I do in this iteration.
>> +template<typename F>
>> + 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?
>> +decompose_conclusions (tree r)
>
> This function is currently never used; I assume that is planned for a later
> patch?
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.
>> +// The following functions will optionally parse a rule if the
>> corresponding
>> +// criteria is satisfied. If not, the parser returns NULL indicating that
>> +// the optional parse taken.
>
> Missing a "was not". And these functions should also take the rule as a
> template argument rather than a function argument. 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.
I ended up rewriting this based on Gaby's comments, but not to use
template arguments.
> There are a bunch of unnecessary whitespace changes in parser.c, including
> this one.
I'll re-generate the patch with -w after cleanups.
>
>> else
>> initializer = NULL_TREE;
>>
>> + // If we're looking at a function declaration, then a
>> requires
>> + // clause may follow the declaration.
>> + tree saved_template_reqs = current_template_reqs;
>
>
> 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.
>> +// 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.
>> +tree
>> +substitute_template_parameters (tree parms, tree args)
>
>
> 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).
This is only used by my homebrew overload resolution for concepts, so
this could go away.
Andrew
