Re: [C++ PATCH] pr 67273 & 79253 lambdas, warnings & ICEs

2017-01-31 Thread Nathan Sidwell 

On 01/31/2017 10:04 AM, Jason Merrill wrote:


Agreed.  As I was suggesting in response to one of Adam's patches, I
think we need to defer creating the closure until f is instantiated;
at that point we can resolve all names from f and so we should be able
to always push to top when instantiating the lambda.


Yup.


Perhaps

if (!fn_context || fn_context != current_function_decl)


That works fine, thanks.   It also makes it clearer that 'nested' must 
be true if !push_to_top (but not vice-versa), which allowed a little 
more simplification.


Committed the attached to trunk.

nathan
--
Nathan Sidwell
2017-01-31  Nathan Sidwell  

	PR c++/67273
	PR c++/79253
	* pt.c: (instantiate_decl): Push to top level when current
	function scope doesn't match.  Only push lmabda scope stack when
	pushing to top.

	PR c++/67273
	PR c++/79253
	* g++.dg/cpp1y/pr67273.C: New.
	* g++.dg/cpp1y/pr79253.C: New.

Index: cp/pt.c
===
--- cp/pt.c	(revision 245066)
+++ cp/pt.c	(working copy)
@@ -22666,20 +22666,21 @@ instantiate_decl (tree d, bool defer_ok,
 	goto out;
 }
 
-  bool nested;
+  bool push_to_top, nested;
   tree fn_context;
   fn_context = decl_function_context (d);
-  nested = (current_function_decl != NULL_TREE);
+  nested = current_function_decl != NULL_TREE;
+  push_to_top = !(nested && fn_context == current_function_decl);
+
   vec omp_privatization_save;
   if (nested)
 save_omp_privatization_clauses (omp_privatization_save);
 
-  if (!fn_context)
+  if (push_to_top)
 push_to_top_level ();
   else
 {
-  if (nested)
-	push_function_context ();
+  push_function_context ();
   cp_unevaluated_operand = 0;
   c_inhibit_evaluation_warnings = 0;
 }
@@ -22756,7 +22757,7 @@ instantiate_decl (tree d, bool defer_ok,
 	block = push_stmt_list ();
   else
 	{
-	  if (LAMBDA_FUNCTION_P (d))
+	  if (push_to_top && LAMBDA_FUNCTION_P (d))
 	{
 	  /* When instantiating a lambda's templated function
 		 operator, we need to push the non-lambda class scope
@@ -22849,9 +22850,9 @@ instantiate_decl (tree d, bool defer_ok,
   /* We're not deferring instantiation any more.  */
   TI_PENDING_TEMPLATE_FLAG (DECL_TEMPLATE_INFO (d)) = 0;
 
-  if (!fn_context)
+  if (push_to_top)
 pop_from_top_level ();
-  else if (nested)
+  else
 pop_function_context ();
 
   if (nested)
Index: testsuite/g++.dg/cpp1y/pr67273.C
===
--- testsuite/g++.dg/cpp1y/pr67273.C	(revision 0)
+++ testsuite/g++.dg/cpp1y/pr67273.C	(working copy)
@@ -0,0 +1,16 @@
+// { dg-do compile { target c++14 } }
+// { dg-additional-options "-Wshadow" }
+
+// pr67273 bogus warning about shadowing.
+
+
+template  void Foo (T &)
+{
+  int ARG = 2;
+  lambda (1);
+}
+
+void Baz ()
+{
+  Foo ([] (auto &) {});
+}
Index: testsuite/g++.dg/cpp1y/pr79253.C
===
--- testsuite/g++.dg/cpp1y/pr79253.C	(revision 0)
+++ testsuite/g++.dg/cpp1y/pr79253.C	(working copy)
@@ -0,0 +1,33 @@
+// { dg-do compile { target c++14 } }
+// PR 79253 ICE instantiating lambda body.
+
+template  struct A;
+template > class B {};
+template  void foo (U, V) { T (0, 0); }
+struct C {};
+template  class F, class>
+void
+bar ()
+{
+  F<0, 0, 0>::baz;
+}
+struct G { int l; };
+template  struct E : C
+{
+  E (int, int) : e (0)
+  {
+auto  = this->m ();
+auto c = [&] { m.l; };
+  }
+  G  ();
+  int e;
+};
+struct D
+{
+  void
+  baz () { bar>; }
+  template  struct F
+  {
+static B<> baz () { foo> (0, 0); }
+  };
+};

Re: [C++ PATCH] pr 67273 & 79253 lambdas, warnings & ICEs

2017-01-31 Thread Jason Merrill 
On Tue, Jan 31, 2017 at 8:31 AM, Nathan Sidwell  wrote:
> On 01/30/2017 03:48 PM, Jason Merrill wrote:
>> Why can't it figure that out for itself?  We should be able to tell
>> whether its containing function is currently open.
>
> It doesn't have sufficient information (but that may not matter, see below).
>
> template  void Foo (T lam)
> {
>   lam (1u); // #1
> }
>
> template 
> void f(T x)
> {
>   auto lam = [](auto x) { return  (x); };
>
>   lam (1); // #2
>   Foo (lam);
> }
>
> void Bar ()
> {
>   f(1);
> }
>
> at #1 and #2 we end up via maybe_instantiate in instantiate_decl (to
> determine the return type when building the CALL_EXPR).  At #1
> current_function_decl is the template Foo, which is not the context of the
> closure type.
>
> At #2 we go via the same path, but current_function_decl is 'f', which is
> the context of the closure type.
>
> It seems wrong to me to push to top in one of those cases but not in the
> other.  Again, absent of generic lambdas, we'd always push to top in these
> circumstances.

Agreed.  As I was suggesting in response to one of Adam's patches, I
think we need to defer creating the closure until f is instantiated;
at that point we can resolve all names from f and so we should be able
to always push to top when instantiating the lambda.

> That said, using the predicate:
>current_function_decl
> == DECL_CONTEXT (TYPE_NAME (CP_DECL_CONTEXT (d)))
> to determine whether a lambda instantiate should push to top or not doesn't
> cause any test failures. (and does resolve the shadowing bug).

Perhaps

if (!fn_context || fn_context != current_function_decl)

?

Jason

Re: [C++ PATCH] pr 67273 & 79253 lambdas, warnings & ICEs

2017-01-31 Thread Nathan Sidwell 

On 01/30/2017 03:48 PM, Jason Merrill wrote:


Why can't it figure that out for itself?  We should be able to tell
whether its containing function is currently open.


It doesn't have sufficient information (but that may not matter, see below).

template  void Foo (T lam)
{
  lam (1u); // #1
}

template 
void f(T x)
{
  auto lam = [](auto x) { return  (x); };

  lam (1); // #2
  Foo (lam);
}

void Bar ()
{
  f(1);
}

at #1 and #2 we end up via maybe_instantiate in instantiate_decl (to 
determine the return type when building the CALL_EXPR).  At #1 
current_function_decl is the template Foo, which is not the context of 
the closure type.


At #2 we go via the same path, but current_function_decl is 'f', which 
is the context of the closure type.


It seems wrong to me to push to top in one of those cases but not in the 
other.  Again, absent of generic lambdas, we'd always push to top in 
these circumstances.


That said, using the predicate:
   current_function_decl
== DECL_CONTEXT (TYPE_NAME (CP_DECL_CONTEXT (d)))
to determine whether a lambda instantiate should push to top or not 
doesn't cause any test failures. (and does resolve the shadowing bug).


In case you're wondering, if we always push to top for a lambda a bunch 
of tests fail.  It seems (at least) the access checking breaks.


nathan

--
Nathan Sidwell

Re: [C++ PATCH] pr 67273 & 79253 lambdas, warnings & ICEs

2017-01-30 Thread Jason Merrill 
On Fri, Jan 27, 2017 at 7:45 AM, Nathan Sidwell  wrote:
> Jason,
> I happened to be working on 67273, noticed a problem with my 77585 fix, and
> coincidentally 79253 got filed, which this also fixes.
>
> In 67253,  Wshadow checking was getting confused when determining the return
> type of an instantiated lambda.
>
> template  void Foo (T &) {
>   int ARG = 2;
>   lambda (1);
> }
>
> void Baz () {
>   Foo ([] (auto &) {});
> }
>
> maybe_instantiate_decl gets called when building the 'lambda (1)' call
> during the instantiation of Foo (to determine its return type).  It goes
> ahead and calls instantiate_decl.  instantiate_decl decides not to push to
> top level at:
>
>   fn_context = decl_function_context (d);
>   ...
>   if (!fn_context)
> push_to_top_level ();
>   else
> ...
>
> because 'decl_function_context' is true (it's 'Baz'), but the current
> function is 'Foo'.  We end up in store_parms thinking we're
> pushing a parm 'ARG' that shadows the local var 'ARG'.  That doesn't result
> in wrong code, but does give a spurious shadowing warning.
>
> Again, this is an artifact of generic lambdas being template members of
> function-scope classes.  Not a thing that exists elsewhere.
>
> Unfortunately, there is a case where we do want to stay at the current level
> -- when we're instantiating the lambda body during instantiation of the
> containing template function.  instantiate_decl must be told in which
> context it's being invoked.

Why can't it figure that out for itself?  We should be able to tell
whether its containing function is currently open.

Jason

[C++ PATCH] pr 67273 & 79253 lambdas, warnings & ICEs

2017-01-27 Thread Nathan Sidwell 

Jason,
I happened to be working on 67273, noticed a problem with my 77585 fix, 
and coincidentally 79253 got filed, which this also fixes.


In 67253,  Wshadow checking was getting confused when determining the 
return type of an instantiated lambda.


template  void Foo (T &) {
  int ARG = 2;
  lambda (1);
}

void Baz () {
  Foo ([] (auto &) {});
}

maybe_instantiate_decl gets called when building the 'lambda (1)' call 
during the instantiation of Foo (to determine its return type).  It goes 
ahead and calls instantiate_decl.  instantiate_decl decides not to push 
to top level at:


  fn_context = decl_function_context (d);
  ...
  if (!fn_context)
push_to_top_level ();
  else
...

because 'decl_function_context' is true (it's 'Baz'), but the current 
function is 'Foo'.  We end up in store_parms thinking 
we're pushing a parm 'ARG' that shadows the local var 'ARG'.  That 
doesn't result in wrong code, but does give a spurious shadowing warning.


Again, this is an artifact of generic lambdas being template members of 
function-scope classes.  Not a thing that exists elsewhere.


Unfortunately, there is a case where we do want to stay at the current 
level -- when we're instantiating the lambda body during instantiation 
of the containing template function.  instantiate_decl must be told in 
which context it's being invoked.


the 77585 problem is:
  if (LAMBDA_FUNCTION_P (d))
{
  /* When instantiating a lambda's templated function
 operator, we need to push the non-lambda class scope
 of the lambda itself so that the nested function
 stack is sufficiently correct to deal with this
 capture.  */
We don't want to do that when we're not pushing to top level (I had 
thought it was just extra work, but 79253 shows it's a correctness problem).


As 'defer_ok' is still an int parm, I did toy with making it tri-valued, 
but went for adding an additional parm, and correcting defer_ok's type 
in the process.  All callers outside of pt.c were already passing true 
or false.


ok?

nathan

--
Nathan Sidwell
2017-01-26  Nathan Sidwell  

	PR c++/67273
	PR c++/79253
	* cp-tree.h (instantiate_decl): Make defer_ok bool, add
	lambda_subst flag.
	* pt.c: Fix instantiate_decl calls to pass true/false not 0/1
	(instantiate_class_template_1): Inform instantiate_decl of
	lambda fn substing.
	(instantiate_decl): Push to top level for lambda fn instanitation,
	except when lambda_subst.

	PR c++/67273
	PR c++/79253
	* g++.dg/cpp1y/pr67273.C: New.
	* g++.dg/cpp1y/pr79253.C: New.

Index: cp/cp-tree.h
===
--- cp/cp-tree.h	(revision 244932)
+++ cp/cp-tree.h	(working copy)
@@ -6182,7 +6182,8 @@ extern void do_decl_instantiation		(tree
 extern void do_type_instantiation		(tree, tree, tsubst_flags_t);
 extern bool always_instantiate_p		(tree);
 extern void maybe_instantiate_noexcept		(tree);
-extern tree instantiate_decl			(tree, int, bool);
+extern tree instantiate_decl			(tree, bool, bool,
+		 bool = false);
 extern int comp_template_parms			(const_tree, const_tree);
 extern bool uses_parameter_packs(tree);
 extern bool template_parameter_pack_p   (const_tree);
Index: cp/pt.c
===
--- cp/pt.c	(revision 244932)
+++ cp/pt.c	(working copy)
@@ -10676,7 +10676,8 @@ instantiate_class_template_1 (tree type)
 	{
 	  /* Set function_depth to avoid garbage collection.  */
 	  ++function_depth;
-	  instantiate_decl (decl, false, false);
+	  instantiate_decl (decl, /*defer_ok=*/false, false,
+/*lambda_subst=*/true);
 	  --function_depth;
 	}
 
@@ -16024,7 +16025,8 @@ tsubst_expr (tree t, tree args, tsubst_f
 	  complete_type (tmp);
 	  for (fn = TYPE_METHODS (tmp); fn; fn = DECL_CHAIN (fn))
 	if (!DECL_ARTIFICIAL (fn))
-	  instantiate_decl (fn, /*defer_ok*/0, /*expl_inst_class*/false);
+	  instantiate_decl (fn, /*defer_ok=*/false,
+/*expl_inst_class=*/false);
 	}
   break;
 
@@ -21941,7 +21943,7 @@ do_decl_instantiation (tree decl, tree s
   check_explicit_instantiation_namespace (result);
   mark_decl_instantiated (result, extern_p);
   if (! extern_p)
-instantiate_decl (result, /*defer_ok=*/1,
+instantiate_decl (result, /*defer_ok=*/true,
 		  /*expl_inst_class_mem_p=*/false);
 }
 
@@ -21979,7 +21981,7 @@ instantiate_class_member (tree decl, int
 {
   mark_decl_instantiated (decl, extern_p);
   if (! extern_p)
-instantiate_decl (decl, /*defer_ok=*/1,
+instantiate_decl (decl, /*defer_ok=*/true,
 		  /*expl_inst_class_mem_p=*/true);
 }
 
@@ -22400,15 +22402,17 @@ maybe_instantiate_noexcept (tree fn)
 }
 
 /* Produce the definition of D, a _DECL generated from a template.  If
-   DEFER_OK is nonzero, then we don't have to actually do the
+   DEFER_OK is true, then we don't have to actually do the