[c++-concepts] code review

[Andrew Sutton]

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