Re: [c++-concepts] code review

[Andrew Sutton]

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:

templatetypename T
void f()
{
  vectorint 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

[c++-concepts] code review

[Jason Merrill]

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, vectree, va_gc 
**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_deferred);
+}
+~ 

Re: [c++-concepts] code review

[Andrew Sutton]

 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, vectree, va_gc
 **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  

Re: [c++-concepts] code review

[Jason Merrill]

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

[Andrew Sutton]

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 ja...@redhat.com 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

[Jason Merrill]

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

[Gabriel Dos Reis]

Jason Merrill ja...@redhat.com 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

[Andrew Sutton]

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 g...@axiomatics.org wrote:
 Jason Merrill ja...@redhat.com 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]

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

[Andrew Sutton]

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 ja...@redhat.com 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

[Jason Merrill]

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

[Gabriel Dos Reis]

Andrew Sutton andrew.n.sut...@gmail.com 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

[Gabriel Dos Reis]

Jason Merrill ja...@redhat.com 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

[Jason Merrill]

On 06/20/2013 11:22 AM, Gabriel Dos Reis wrote:

Jason Merrill ja...@redhat.com 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

[Gabriel Dos Reis]

Jason Merrill ja...@redhat.com writes:

| On 06/20/2013 11:22 AM, Gabriel Dos Reis wrote:
|  Jason Merrill ja...@redhat.com 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

[Andrew Sutton]

 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]

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

[Jason Merrill]

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

[Jason Merrill]

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  ja...@redhat.com

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

[Andrew Sutton]

 +// 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

[Gabriel Dos Reis]

On Wed, Jun 19, 2013 at 9:21 AM, Jason Merrill ja...@redhat.com 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  ja...@redhat.com

 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

[Andrew Sutton]

 2. I left the cstdlib include in system.h, because removing it will
 result in errors due to an inclusion of algorithm. It's probable
 that both can be removed, but I didn't test it with this patch.


 You mean you don't need algorithm anymore in logic.cc?  I think we want
 the cstdlib 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 see. That is cleaner.


 +  // FIXME: we should be checking the constraints, not just
 +  // 

Re: [c++-concepts] code review

[Jason Merrill]

On 06/17/2013 02:10 PM, Andrew Sutton wrote:

You mean you don't need algorithm anymore in logic.cc?  I think we want
the cstdlib 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 class T struct A {
  T t;
 requires AddableT()
  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

[Gabriel Dos Reis]

On Mon, Jun 17, 2013 at 2:20 PM, Jason Merrill ja...@redhat.com wrote:

 I have not thought deeply about constrained friend declarations. What
 would a friend temploid look like?


 I was thinking something like

 template class T struct A {
   T t;
  requires AddableT()
   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

[Jason Merrill]

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 cstdlib include in system.h, because removing it will
result in errors due to an inclusion of algorithm. It's probable
that both can be removed, but I didn't test it with this patch.


You mean you don't need algorithm anymore in logic.cc?  I think we 
want the cstdlib 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 

Re: [c++-concepts] code review

[Gabriel Dos Reis]

On Fri, Jun 14, 2013 at 8:40 PM, Jason Merrill ja...@redhat.com 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

[Gabriel Dos Reis]

On Mon, Jun 10, 2013 at 2:30 PM, Jason Merrill ja...@redhat.com 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

[Jason Merrill]

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

[Andrew Sutton]

 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

[Jason Merrill]

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

[Andrew Sutton]

 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

[Jason Merrill]

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

[Andrew Sutton]

 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

[Jason Merrill]

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

[Diego Novillo]

On 2013-06-09 20:34 , Gabriel Dos Reis wrote:

So, my advice is for GCC source code to forget about the cxxx
headers for the  most part.  I can see an instance where cmath or cstring
would make a difference but given point (1) above, no it doesn't.
Just use the traditional xxx.h 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

[Jason Merrill]

On 06/09/2013 08:34 PM, Gabriel Dos Reis wrote:

I strongly suggest prefering stdlib.h over cstdlib for GCC source code
base.


The problem is that including stdlib.h does not define 
_GLIBCXX_CSTDLIB, so if one of the C++ library headers includes 
cstdlib 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

[Jason Merrill]

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

[Jason Merrill]

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

[Lawrence Crowl]

On 6/10/13, Diego Novillo dnovi...@google.com wrote:
 On 2013-06-09 20:34 , Gabriel Dos Reis wrote:
  So, my advice is for GCC source code to forget about the cxxx
  headers for the most part.  I can see an instance where cmath
  or cstring would make a difference but given point (1) above,
  no it doesn't.  Just use the traditional xxx.h 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

[Oleg Endo]

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

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 cstdlib 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

[Gabriel Dos Reis]

On Sun, Jun 9, 2013 at 3:34 PM, Oleg Endo oleg.e...@t-online.de 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 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.


 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 cstdlib 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 stdlib.h over cstdlib 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 cxxx
headers for the  most part.  I can see an instance where cmath or cstring
would make a difference but given point (1) above, no it doesn't.
Just use the traditional xxx.h 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

[Gabriel Dos Reis]

On Sat, Jun 8, 2013 at 8:34 AM, Andrew Sutton andrew.n.sut...@gmail.com wrote:

 templateterm_list (*proj)(proof_goal)
 tree
 extract_goals (proof_state s)
 ...
  return extract_goalsassumptions(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

[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?


 templateterm_list (*proj)(proof_goal)
 tree
 extract_goals (proof_state s)
 ...
  return extract_goalsassumptions(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

[c++-concepts] code review

[Jason Merrill]

Hi, I'm finally going through the current code on the branch, sorry for 
the delay.



* 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.



+  /* 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;
   vecqualified_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:



In general constraints are directly associated with a template decl. For 
example:

templateArithmetic T
  class complex;

The Arithmetic constraint is associated with the template decl. However, this 
doesn't seem to work with partial specializations:

templateReal T
  struct complexT { ... };

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 

Re: [c++-concepts] code review

[Andrew Sutton]

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;
vecqualified_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 

Re: [c++-concepts] code review

[Jason Merrill]

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



+templatetypename 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

templateterm_list (*proj)(proof_goal)
tree
extract_goals (proof_state s)
...
 return extract_goalsassumptions(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