Re: [c++-concepts] code review

[Jason Merrill <jason@redhat.com>]

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

I was thinking something like

template<term_list& (*proj)(proof_goal&)>
tree
extract_goals (proof_state& s)
...
  return extract_goals<assumptions>(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