[PR,c++/83160] local ref to capture

Message ID 4cde21ec-f32a-203c-c4b6-756e35f03738@acm.org
State New
Headers show
Series
  • [PR,c++/83160] local ref to capture
Related show

Commit Message

Nathan Sidwell Jan. 12, 2018, 7:11 p.m.
Jason,
this fixes 83160, where we complain about not having an lvalue in things 
like:

void foo () {
   const int a = 0;
   [&a] () {
     const int &b = a;  // here
   };
}

The problem is that we in convert_like_real we have ref_bind->identity 
conversions, and the identity conversion calls 'mark_rvalue_use'.  For a 
regular rvalue use of a 'const int' VAR_DECL, we return the VAR_DECL -- 
not collapse it to the initialized value.  However, for captures, there 
is code to look through the capture when rvalue_p is true.  We end up 
looking through the lambda capture, through the captured 'a' and to its 
initializer.   oops.

Calling mark_lvalue_use instead is also wrong, because the identity conv 
may of course be being applied to an rvalue, and we end up giving an 
equally wrong error in the opposite case.  Dealing with ck_identity this 
way sees wrong -- context determines whether this is an rvalue or lvalue 
use.

I think the solution is for the identity conv to know whether it's going 
to be the subject of a direct reference binding and call mark_lvalue_use 
in tha case.  There's 2 issues with that:

1) mark_lvalue_use isn't quite right because we want to specify 
reject_builtin to the inner mark_use call.  (I didn't try changing 
mark_lvalue_use to pass true, I suspected it'd break actual calls of 
builtins).  Fixed by making mark_use externally reachable, and passing 
in an appropriate 'rvalue_p' parm directly.  I think your recent patch 
to select between the two marking fns may be neater using this entry point?

2) I modify direct_reference_binding to look at the incoming conv, and 
if it is ck_identity set that conv's rvaluedness_matches_p flag.  Then 
deply that flag to determine the arg in #1.  'rvaluedness_matches_p' 
seemed the least worst existing flag to press into service here.

WDYT?

nathan

Comments

Jason Merrill Jan. 17, 2018, 6:44 p.m. | #1
On Fri, Jan 12, 2018 at 2:11 PM, Nathan Sidwell <nathan@acm.org> wrote:
> Jason,
> this fixes 83160, where we complain about not having an lvalue in things
> like:
>
> void foo () {
>   const int a = 0;
>   [&a] () {
>     const int &b = a;  // here
>   };
> }
>
> The problem is that we in convert_like_real we have ref_bind->identity
> conversions, and the identity conversion calls 'mark_rvalue_use'.  For a
> regular rvalue use of a 'const int' VAR_DECL, we return the VAR_DECL -- not
> collapse it to the initialized value.  However, for captures, there is code
> to look through the capture when rvalue_p is true.  We end up looking
> through the lambda capture, through the captured 'a' and to its initializer.
> oops.
>
> Calling mark_lvalue_use instead is also wrong, because the identity conv may
> of course be being applied to an rvalue, and we end up giving an equally
> wrong error in the opposite case.  Dealing with ck_identity this way sees
> wrong -- context determines whether this is an rvalue or lvalue use.
>
> I think the solution is for the identity conv to know whether it's going to
> be the subject of a direct reference binding and call mark_lvalue_use in tha
> case.  There's 2 issues with that:
>
> 1) mark_lvalue_use isn't quite right because we want to specify
> reject_builtin to the inner mark_use call.  (I didn't try changing
> mark_lvalue_use to pass true, I suspected it'd break actual calls of
> builtins).  Fixed by making mark_use externally reachable, and passing in an
> appropriate 'rvalue_p' parm directly.  I think your recent patch to select
> between the two marking fns may be neater using this entry point?
>
> 2) I modify direct_reference_binding to look at the incoming conv, and if it
> is ck_identity set that conv's rvaluedness_matches_p flag.  Then deply that
> flag to determine the arg in #1.  'rvaluedness_matches_p' seemed the least
> worst existing flag to press into service here.

This makes sense to me.  But I think we'd want also that flag set on
the ck_identity inside the ck_base that direct_reference_binding
creates, so setting it first rather than in an else.

Jason
Nathan Sidwell Jan. 18, 2018, 11:53 a.m. | #2
On 01/17/2018 01:44 PM, Jason Merrill wrote:

> This makes sense to me.  But I think we'd want also that flag set on
> the ck_identity inside the ck_base that direct_reference_binding
> creates, so setting it first rather than in an else.

Ah yes, you spotted the bit I failed to mention.  (I think we get away 
with it because class objects always have sufficient lvalueness, but why 
lie?)

nathan

Patch

2018-01-12  Nathan Sidwell  <nathan@acm.org>

	PR c++/83160
	* cp-tree.h (mark_use): Declare.
	* expr.c (mark_use): Make extern.
	* call.c (direct_reference_binding): Set inner conv's
	rvaluedness_matches_p, if it is an identity.
	(convert_like_real): Mark lvalue or rvalue use for identity as
	rvaledness_matches_p demands.

	PR c++/83160
	* g++.dg/cpp0x/pr83160.C: New.

Index: cp/call.c
===================================================================
--- cp/call.c	(revision 256593)
+++ cp/call.c	(working copy)
@@ -102,7 +102,8 @@  struct conversion {
      being bound to an lvalue expression or an rvalue reference is
      being bound to an rvalue expression.  If KIND is ck_rvalue,
      true when we are treating an lvalue as an rvalue (12.8p33).  If
-     KIND is ck_base, always false.  */
+     KIND is ck_base, always false.  If ck_identity, we will be
+     binding a reference directly.  */
   BOOL_BITFIELD rvaluedness_matches_p: 1;
   BOOL_BITFIELD check_narrowing: 1;
   /* The type of the expression resulting from the conversion.  */
@@ -1501,6 +1502,10 @@  direct_reference_binding (tree type, con
 	 That way, convert_like knows not to generate a temporary.  */
       conv->need_temporary_p = false;
     }
+  else if (conv->kind == ck_identity)
+    /* Mark the identity conv as to not decay to rvalue.  */
+    conv->rvaluedness_matches_p = true;
+
   return build_conv (ck_ref_bind, type, conv);
 }
 
@@ -6800,7 +6805,9 @@  convert_like_real (conversion *convs, tr
 	  else
 	    gcc_unreachable ();
 	}
-      expr = mark_rvalue_use (expr);
+      expr = mark_use (expr, /*rvalue_p=*/!convs->rvaluedness_matches_p,
+		       /*read_p=*/true, UNKNOWN_LOCATION,
+		       /*reject_builtin=*/true);
 
       if (type_unknown_p (expr))
 	expr = instantiate_type (totype, expr, complain);
Index: cp/cp-tree.h
===================================================================
--- cp/cp-tree.h	(revision 256593)
+++ cp/cp-tree.h	(working copy)
@@ -6328,6 +6328,9 @@  extern tree create_try_catch_expr
 
 /* in expr.c */
 extern tree cplus_expand_constant		(tree);
+extern tree mark_use (tree expr, bool rvalue_p, bool read_p,
+		      location_t = UNKNOWN_LOCATION,
+		      bool reject_builtin = true);
 extern tree mark_rvalue_use			(tree,
                                                  location_t = UNKNOWN_LOCATION,
                                                  bool reject_builtin = true);
Index: cp/expr.c
===================================================================
--- cp/expr.c	(revision 256593)
+++ cp/expr.c	(working copy)
@@ -89,7 +89,7 @@  cplus_expand_constant (tree cst)
 /* We've seen an actual use of EXPR.  Possibly replace an outer variable
    reference inside with its constant value or a lambda capture.  */
 
-static tree
+tree
 mark_use (tree expr, bool rvalue_p, bool read_p,
 	  location_t loc /* = UNKNOWN_LOCATION */,
 	  bool reject_builtin /* = true */)
Index: testsuite/g++.dg/cpp0x/pr83160.C
===================================================================
--- testsuite/g++.dg/cpp0x/pr83160.C	(revision 0)
+++ testsuite/g++.dg/cpp0x/pr83160.C	(working copy)
@@ -0,0 +1,33 @@ 
+// { dg-do run { target c++11 } }
+// PR c++/83160 failed to capture as lvalue
+
+int main ()
+{
+  const int a = 0;
+
+  if (![&a] (const int *p)
+      {
+	const int &b = a;
+	// We should bind to the outer a
+	return &b == p;
+      } (&a))
+    return 1;
+
+  if (![&] (const int *p)
+      {
+	const int &b = a;
+	// We should bind to the outer a
+	return &b == p;
+      } (&a))
+    return 2;
+
+  if ([=] (const int *p)
+      {
+	const int &b = a;
+	// We should bind to the captured instance
+	return &b == p;
+      }(&a))
+    return 3;
+
+  return 0;
+}