[PATCH] [PR c++/61636 and c++/64382] Fix default capture in generic lambdas

[PATCH] [PR c++/61636 and c++/64382] Fix default capture in generic lambdas

[Adam Butcher]

Hi Jason,

I finally scraped some time together to look into these two generic
lambda default capture bugs and believe I have a solution.  Still have
to run tests but I thought I'd get these out for your perusal whilst I
rebase onto origin/master and run the testsuite.

Cheers,
Adam

[PATCH 1/2] PR c++/61636 * cp/parser.c (cp_parser_postfix_expression): Resolve default captured 'this' early for generic lambdas.

[Adam Butcher]

	PR c++/61636
	* g++.dg/cpp1y/pr61636.C: New test.
---
 gcc/cp/parser.c                      | 16 ++++++++++++++++
 gcc/testsuite/g++.dg/cpp1y/pr61636.C | 19 +++++++++++++++++++
 2 files changed, 35 insertions(+)
 create mode 100644 gcc/testsuite/g++.dg/cpp1y/pr61636.C

diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c
index 4d6b479..ac91976 100644
--- a/gcc/cp/parser.c
+++ b/gcc/cp/parser.c
@@ -6321,6 +6321,22 @@ cp_parser_postfix_expression (cp_parser *parser, bool address_p, bool cast_p,
 		tree instance = TREE_OPERAND (postfix_expression, 0);
 		tree fn = TREE_OPERAND (postfix_expression, 1);
 
+		/* For generic lambdas, resolve default captured 'this' now.  */
+		if (processing_template_decl
+		    && is_dummy_object (instance)
+		    && current_class_type
+		    && CLASSTYPE_LAMBDA_EXPR (current_class_type))
+		  if (tree callop = lambda_function (current_class_type))
+		    if (DECL_TEMPLATE_INFO (callop)
+			&& (DECL_TEMPLATE_RESULT (DECL_TI_TEMPLATE (callop))
+			    == callop)
+			&& TREE_TYPE (instance) != current_class_type
+			&& DERIVED_FROM_P (TREE_TYPE (instance),
+					   current_nonlambda_class_type ()))
+		      TREE_OPERAND (postfix_expression, 0)
+			= instance
+			= maybe_resolve_dummy (instance, true);
+
 		if (processing_template_decl
 		    && (type_dependent_expression_p (instance)
 			|| (!BASELINK_P (fn)
diff --git a/gcc/testsuite/g++.dg/cpp1y/pr61636.C b/gcc/testsuite/g++.dg/cpp1y/pr61636.C
new file mode 100644
index 0000000..5694675
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp1y/pr61636.C
@@ -0,0 +1,19 @@
+// PR c++/61636
+// { dg-do compile { target c++1y } }
+
+struct X
+{
+   void f(int) {}
+
+   auto defer_f()
+   {
+      return [&] (auto x) {
+         return f(x);
+      };
+   }
+};
+
+int main()
+{
+   X().defer_f()(2);
+}
-- 
2.3.5

Re: [PATCH 1/2] PR c++/61636

[Jason Merrill]

On 04/09/2015 11:31 PM, Adam Butcher wrote:
> +		/* For generic lambdas, resolve default captured 'this' now.  */
> +		if (processing_template_decl
> +		    && is_dummy_object (instance)
> +		    && current_class_type
> +		    && CLASSTYPE_LAMBDA_EXPR (current_class_type))
> +		  if (tree callop = lambda_function (current_class_type))
> +		    if (DECL_TEMPLATE_INFO (callop)
> +			&& (DECL_TEMPLATE_RESULT (DECL_TI_TEMPLATE (callop))
> +			    == callop)
> +			&& TREE_TYPE (instance) != current_class_type
> +			&& DERIVED_FROM_P (TREE_TYPE (instance),
> +					   current_nonlambda_class_type ()))
> +		      TREE_OPERAND (postfix_expression, 0)
> +			= instance
> +			= maybe_resolve_dummy (instance, true);

This isn't quite right.  We don't want to capture 'this' any time we see 
a member function call, as overload resolution might choose a static 
member function that doesn't need 'this'.  The special handling we want 
is for the case where the call depends on a generic lambda parameter, in 
which case we capture 'this' because the call "names [this] in a 
potentially-evaluated expression (3.2) where the enclosing 
full-expression depends on a generic lambda parameter declared within 
the reaching scope of the lambda-expression."

Jason

Re: [PATCH 1/2] PR c++/61636

[Adam Butcher]

On 2015-04-17 20:58, Jason Merrill wrote:
> On 04/09/2015 11:31 PM, Adam Butcher wrote:
>> +		/* For generic lambdas, resolve default captured 'this' now.  */
>>
> This isn't quite right.  We don't want to capture 'this' any time we
> see a member function call, as overload resolution might choose a
> static member function that doesn't need 'this'.  The special 
> handling
> we want is for the case where the call depends on a generic lambda
> parameter, in which case we capture 'this' because the call "names
> [this] in a potentially-evaluated expression (3.2) where the 
> enclosing
> full-expression depends on a generic lambda parameter declared within
> the reaching scope of the lambda-expression."
>
Good point.  I'll look into it.  So for a nullary member call we will 
always capture 'this', but for N-ary, we only capture if we find one of 
the lambda's parameters (or a parameter from an enclosing generic 
lambda?) in the call's arguments right?

Cheers,
Adam

Re: [PATCH 1/2] PR c++/61636

[Adam Butcher]

On 2015-04-17 22:06, Adam Butcher wrote:
> On 2015-04-17 20:58, Jason Merrill wrote:
>> On 04/09/2015 11:31 PM, Adam Butcher wrote:
>>> +		/* For generic lambdas, resolve default captured 'this' now.  */
>>>
>> This isn't quite right.  We don't want to capture 'this' any time we
>> see a member function call, as overload resolution might choose a
>> static member function that doesn't need 'this'.  The special 
>> handling
>> we want is for the case where the call depends on a generic lambda
>> parameter, in which case we capture 'this' because the call "names
>> [this] in a potentially-evaluated expression (3.2) where the 
>> enclosing
>> full-expression depends on a generic lambda parameter declared 
>> within
>> the reaching scope of the lambda-expression."
>>
> Good point.  I'll look into it.  So for a nullary member call we will
> always capture 'this', but for N-ary, we only capture if we find one
> of the lambda's parameters (or a parameter from an enclosing generic
> lambda?) in the call's arguments right?
>

Test like this?

/* { dg-do run { target c++14 } }  */
/* { dg-final { scan-assembler-not "..." } }  */

struct X
{
   int f (int, double) { return 255; }
   static int f (int, int) { return 65535; }

   auto m1 ()
   {
     return [=] (auto a) {
       return f (7, a);
     };
   }

   auto m2 ()
   {
     return [=] (auto a) {
       return f (9, 10) + a;
     };
   }
};

#include <cassert>

int main()
{
   X x;
   assert (x.m1 () (42.0) == 255);
   assert (x.m1 () (42) == 65535);
   assert (x.m2 () (42.0) == (65535 + 42));
   assert (x.m2 () (42) == (65535 + 42));
   assert (sizeof x.m2 () < sizeof x.m1 ());
}

Re: [PATCH 1/2] PR c++/61636

[Adam Butcher]

On 2015-04-18 18:53, Adam Butcher wrote:
>
> Test like this?
>
> /* { dg-do run { target c++14 } }  */
> /* { dg-final { scan-assembler-not "..." } }  */
>
> struct X
> {
>   int f (int, double) { return 255; }
>   static int f (int, int) { return 65535; }
>
>   auto m1 ()
>   {
>     return [=] (auto a) {
>       return f (7, a);
>     };
>   }
>
>   auto m2 ()
>   {
>     return [=] (auto a) {
>       return f (9, 10) + a;
>     };
>   }

And this:

   auto m3 ()
   {
     return [=] (auto a) {
       return f (11, 12.0) + a;
     };
   }

Currently we don't capture 'this' here despite the call not being 
dependent on any lambda parameter and resolving to a non-static member 
function.

I think I need to resolve a member call within a generic lambda as if 
it were not in template context to determine whether it unambiguously 
resolves to a static member function.  If it does, then no capture 
required.  Otherwise, 'this' should be captured because either a) the 
call is to a non-static member function without any dependent parameters 
or b) because it may resolve to a non-static member function at callop 
instantiate time.

No sure whether I can do all this at my current patch site in 
cp_parser_postfix_expression or whether it needs to be later.

Adam

Re: [PATCH 1/2] PR c++/61636

[Jason Merrill]

On 04/19/2015 03:03 PM, Adam Butcher wrote:
> I think I need to resolve a member call within a generic lambda as if it
> were not in template context to determine whether it unambiguously
> resolves to a static member function.  If it does, then no capture
> required.  Otherwise, 'this' should be captured because either a) the
> call is to a non-static member function without any dependent parameters
> or b) because it may resolve to a non-static member function at callop
> instantiate time.

Right.

> No sure whether I can do all this at my current patch site in
> cp_parser_postfix_expression or whether it needs to be later.

I think it needs to be later, when the context of the lambda is fully 
instantiated, so that any other dependencies are resolved.  Perhaps this 
means that we need to partially instantiate the (generic) lambda body...

This is reminding me of bug 47226, though I'm not sure the fixes need to 
coordinate.

Jason

Re: [PATCH 1/2] ?PR c++/61636

[Marek Polacek]

On Sat, Apr 18, 2015 at 06:53:28PM +0100, Adam Butcher wrote:
> Test like this?
> 
> /* { dg-do run { target c++14 } }  */
> /* { dg-final { scan-assembler-not "..." } }  */

What is this dg-final supposed to do here?

	Marek

Re: [PATCH 1/2] ?PR c++/61636

[Adam Butcher]

On 2015-04-20 11:25, Marek Polacek wrote:
> On Sat, Apr 18, 2015 at 06:53:28PM +0100, Adam Butcher wrote:
>> Test like this?
>>
>> /* { dg-do run { target c++14 } }  */
>> /* { dg-final { scan-assembler-not "..." } }  */
>
> What is this dg-final supposed to do here?

It was a placeholder for making sure that, once fixed, the lambda 
depending on only the static member function does not capture 'this'.  I 
didn't get around to seeing whether that was possible from the asm 
output and if it were, what symbol I should check for the absence of.  
For now I have sizeof assertions but I suppose I could use an sfinae 
test for existence (or otherwise in this case) of the placeholder 
'__this'.

[PATCH 2/2] PR c++/64382 * cp/parser.c (parsing_default_capturing_generic_lambda_in_template): New function. * cp/cp-tree.h: Declare it. * cp/semantics.c (finish_id_expression): Resolve names within a default capturing generic lambda defined within a template prior to instantiation to allow for captures to be added to the closure type.

[Adam Butcher]

	PR c++/64382
	* g++.dg/cpp1y/pr64382.C: New test.
---
 gcc/cp/cp-tree.h                     |  1 +
 gcc/cp/parser.c                      | 21 +++++++++++++++++++++
 gcc/cp/semantics.c                   |  8 +++++---
 gcc/testsuite/g++.dg/cpp1y/pr64382.C | 23 +++++++++++++++++++++++
 4 files changed, 50 insertions(+), 3 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/cpp1y/pr64382.C

diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
index 2a904a5..484f352 100644
--- a/gcc/cp/cp-tree.h
+++ b/gcc/cp/cp-tree.h
@@ -5645,6 +5645,7 @@ extern bool maybe_clone_body			(tree);
 /* In parser.c */
 extern tree cp_convert_range_for (tree, tree, tree, bool);
 extern bool parsing_nsdmi (void);
+extern bool parsing_default_capturing_generic_lambda_in_template (void);
 extern void inject_this_parameter (tree, cp_cv_quals);
 
 /* in pt.c */
diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c
index ac91976..74b55df 100644
--- a/gcc/cp/parser.c
+++ b/gcc/cp/parser.c
@@ -18339,6 +18339,27 @@ parsing_nsdmi (void)
   return false;
 }
 
+/* Return true iff our current scope is a default capturing generic lambda
+   defined within a template.  */
+
+bool
+parsing_default_capturing_generic_lambda_in_template (void)
+{
+  if (processing_template_decl && current_class_type)
+    if (tree lam = CLASSTYPE_LAMBDA_EXPR (current_class_type))
+      if (LAMBDA_EXPR_DEFAULT_CAPTURE_MODE (lam) != CPLD_NONE)
+	if (tree callop = lambda_function (lam))
+	  if (DECL_TEMPLATE_INFO (callop)
+	      && (DECL_TEMPLATE_RESULT (DECL_TI_TEMPLATE (callop))
+		  == callop)
+	      && ((current_nonlambda_class_type ()
+		   && CLASSTYPE_TEMPLATE_INFO (current_nonlambda_class_type ()))
+		  || ((current_nonlambda_function ()
+		       && DECL_TEMPLATE_INFO (current_nonlambda_function ())))))
+	    return true;
+  return false;
+}
+
 /* Parse a late-specified return type, if any.  This is not a separate
    non-terminal, but part of a function declarator, which looks like
 
diff --git a/gcc/cp/semantics.c b/gcc/cp/semantics.c
index 74af7e8..f1ab183 100644
--- a/gcc/cp/semantics.c
+++ b/gcc/cp/semantics.c
@@ -3450,9 +3450,11 @@ finish_id_expression (tree id_expression,
 	    }
 	}
 
-      /* If the name was dependent on a template parameter, we will
-	 resolve the name at instantiation time.  */
-      if (dependent_p)
+      /* If the name was dependent on a template parameter and we're not in a
+	 default capturing generic lambda within a template, we will resolve the
+	 name at instantiation time.  */
+      if (dependent_p
+	  && !parsing_default_capturing_generic_lambda_in_template ())
 	{
 	  /* Create a SCOPE_REF for qualified names, if the scope is
 	     dependent.  */
diff --git a/gcc/testsuite/g++.dg/cpp1y/pr64382.C b/gcc/testsuite/g++.dg/cpp1y/pr64382.C
new file mode 100644
index 0000000..ff552ac
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp1y/pr64382.C
@@ -0,0 +1,23 @@
+// PR c++/64382
+// { dg-do compile { target c++1y } }
+
+template<typename T>
+struct my_queue
+{
+  void push(T)
+  {
+  }
+  void ice()
+  {
+    auto L = [=](auto &&v) {
+      push(v);
+    };
+    trav(L);
+  }
+  template<typename F>
+  void trav(F &&f)
+  {
+    f(T());
+  }
+};
+template struct my_queue<int>;
-- 
2.3.5

Re: [PATCH 2/2] PR c++/64382 * cp/parser.c (parsing_default_capturing_generic_lambda_in_template): New function. * cp/cp-tree.h: Declare it. * cp/semantics.c (finish_id_expression): Resolve names within a default capturing generic lambda defined within a template prior to instantiation to allow for captures to be added to the closure type.

[Marek Polacek]

On Fri, Apr 10, 2015 at 04:31:34AM +0100, Adam Butcher wrote:
> +/* Return true iff our current scope is a default capturing generic lambda
> +   defined within a template.  */
> +
> +bool
> +parsing_default_capturing_generic_lambda_in_template (void)
> +{
> +  if (processing_template_decl && current_class_type)
> +    if (tree lam = CLASSTYPE_LAMBDA_EXPR (current_class_type))
> +      if (LAMBDA_EXPR_DEFAULT_CAPTURE_MODE (lam) != CPLD_NONE)
> +	if (tree callop = lambda_function (lam))
> +	  if (DECL_TEMPLATE_INFO (callop)
> +	      && (DECL_TEMPLATE_RESULT (DECL_TI_TEMPLATE (callop))
> +		  == callop)
> +	      && ((current_nonlambda_class_type ()
> +		   && CLASSTYPE_TEMPLATE_INFO (current_nonlambda_class_type ()))
> +		  || ((current_nonlambda_function ()
> +		       && DECL_TEMPLATE_INFO (current_nonlambda_function ())))))
> +	    return true;
> +  return false;
> +}
> +

Eek, why not &&s rather than cascading ifs?

> +++ b/gcc/testsuite/g++.dg/cpp1y/pr64382.C
> @@ -0,0 +1,23 @@
> +// PR c++/64382
> +// { dg-do compile { target c++1y } }

I think better use c++14 now.

	Marek

Re: [PATCH 2/2] PR c++/64382 * cp/parser.c (parsing_default_capturing_generic_lambda_in_template): New function. * cp/cp-tree.h: Declare it. * cp/semantics.c (finish_id_expression): Resolve names within a default capturing generic lambda defined within a template prior to instantiation to allow for captures to be added to the closure type.

[Jakub Jelinek]

On Tue, Apr 14, 2015 at 09:16:32AM +0200, Marek Polacek wrote:
> On Fri, Apr 10, 2015 at 04:31:34AM +0100, Adam Butcher wrote:
> > +/* Return true iff our current scope is a default capturing generic lambda
> > +   defined within a template.  */
> > +
> > +bool
> > +parsing_default_capturing_generic_lambda_in_template (void)
> > +{
> > +  if (processing_template_decl && current_class_type)
> > +    if (tree lam = CLASSTYPE_LAMBDA_EXPR (current_class_type))
> > +      if (LAMBDA_EXPR_DEFAULT_CAPTURE_MODE (lam) != CPLD_NONE)
> > +	if (tree callop = lambda_function (lam))
> > +	  if (DECL_TEMPLATE_INFO (callop)
> > +	      && (DECL_TEMPLATE_RESULT (DECL_TI_TEMPLATE (callop))
> > +		  == callop)
> > +	      && ((current_nonlambda_class_type ()
> > +		   && CLASSTYPE_TEMPLATE_INFO (current_nonlambda_class_type ()))
> > +		  || ((current_nonlambda_function ()
> > +		       && DECL_TEMPLATE_INFO (current_nonlambda_function ())))))
> > +	    return true;
> > +  return false;
> > +}
> > +
> 
> Eek, why not &&s rather than cascading ifs?

Perhaps because of the lam / callop declarations.
Even a large && is not that readable, I'd say best would be to just use
separate ifs with return false.

  if (!processing_template_decl || !current_class_type)
    return false;

  tree lam = CLASSTYPE_LAMBDA_EXPR (current_class_type);
  if (!lam || LAMBDA_EXPR_DEFAULT_CAPTURE_MODE (lam) == CPLD_NONE)
    return false;

  tree callop = lambda_function (lam);
  if (!callop)
    return false;

...

	Jakub

Re: [PATCH 2/2] PR c++/64382 * cp/parser.c (parsing_default_capturing_generic_lambda_in_template): New function. * cp/cp-tree.h: Declare it. * cp/semantics.c (finish_id_expression): Resolve names within a default capturing generic lambda defined within a template prior to instantiation to allow for captures to be added to the closure type.

[Adam Butcher]

On 2015-04-14 8:26, Jakub Jelinek wrote:
> I'd say best would be to just use separate ifs with return false.
>
Done.

Re: [PATCH 2/2] PR c++/64382 * cp/parser.c (parsing_default_capturing_generic_lambda_in_template)...

[Adam Butcher]

On 2015-04-10 4:31, Adam Butcher wrote:
> +parsing_default_capturing_generic_lambda_in_template (void)
>
I'm wondering whether we should cache this as a bool on the parser.  
Currently it is called once per id-expression and will always return the 
same result for any given lambda.