Fix PR c++/71546 - lambda capture fails with "was not declared in this scope"

Message ID CAK0fzjoPVd+SDieAcGjyz3fBp3gXiCoZD0=DCGAQVSM4+L81vQ@mail.gmail.com
State New
Headers show
Series
  • Fix PR c++/71546 - lambda capture fails with "was not declared in this scope"
Related show

Commit Message

Håkon Sandsmark Feb. 27, 2018, 4:42 p.m.
Hi GCC developers,

I have attached a proposed patch for fixing PR c++/71546 - lambda
capture fails with "was not declared in this scope".

The patch clears the parser scope after each lambda capture in
cp_parser_lambda_introducer in parser.c. This is based on the
following observations:

Comment about cp_parser::scope in parse.h:
    "This value is not cleared automatically after a name is looked
    up, so we must be careful to clear it before starting a new look
    up sequence.  (If it is not cleared, then `X::Y' followed by `Z'
    will look up `Z' in the scope of `X', rather than the current
    scope.)"

C++14 standard draft N4140 § 5.1.2 paragraph 10:
    "The identifier in a simple-capture is looked up using
    the usual rules for unqualified name lookup (3.4.1);
    each such lookup shall find an entity."

I have compared the test results from a pristine build (with test case
from PR added) with a bootstrapped build with my patch applied (using
x86_64-linux). This is the output I got from the compare_tests tool:

    $ gcc/contrib/compare_tests gcc-pristine-build gcc-patched-build
    # Comparing directories
    ## Dir1=gcc-pristine-build: 6 sum files
    ## Dir2=gcc-patched-build: 6 sum files

    # Comparing 6 common sum files
    ## /bin/sh gcc/contrib/compare_tests  /tmp/gxx-sum1.95415
/tmp/gxx-sum2.95415
    Tests that now work, but didn't before:

    g++.dg/cpp1y/pr71546.C  -std=gnu++14 (test for excess errors)

    # No differences found in 6 common sum files

2018-02-27  Håkon Sandsmark  <hsandsmark@gmail.com>

    * parser.c (cp_parser_lambda_introducer): Clear scope after
      each lambda capture.

    * g++.dg/cpp1y/pr71546.C: New test.

Comments

Paolo Carlini Feb. 27, 2018, 6:06 p.m. | #1
Hi,

I only have a simple comment about the testcase:

On 27/02/2018 17:42, Håkon Sandsmark wrote:
> +++ gcc/testsuite/g++.dg/cpp1y/pr71546.C

> @@ -0,0 +1,11 @@

> +// PR c++/71546

> +// { dg-do compile { target c++14 } }

> +// { dg-options "" }

> +

> +#include <memory>

> +

> +int main()

> +{

> +  int x1;

> +  [e = std::make_shared <int> (), x1]() {};

> +}

Instead of including the whole <memory>, shall we use something like:

namespace std { template<typename> struct make_shared { }; }

int main()
{
   int x1;
   [e = std::make_shared <int> (), x1]() {};
}

???

Thanks,
Paolo
Paolo Carlini Feb. 27, 2018, 6:15 p.m. | #2
.. or even:

namespace n { struct make_shared { }; }

int main()
{
   int x1;
   [e = n::make_shared (), x1]() {};
}

I.e., I don't think the fact that std::make_shared is a template plays a 
specific role here.

Paolo.
Håkon Sandsmark Feb. 27, 2018, 6:51 p.m. | #3
Hi,

Thanks for the feedback. I chose to take the example from the bug
report verbatim as the test case.

However, I agree it makes sense to have the simplest possible test
case that reproduces the issue. Here is an updated patch.

2018-02-27  Håkon Sandsmark  <hsandsmark@gmail.com>

    PR c++/71546 - lambda capture fails with "was not declared in this scope"
    * parser.c (cp_parser_lambda_introducer): Clear scope after
      each lambda capture.
    * g++.dg/cpp1y/pr71546.C: New test.

2018-02-27 19:15 GMT+01:00 Paolo Carlini <paolo.carlini@oracle.com>:
> .. or even:

>

> namespace n { struct make_shared { }; }

>

> int main()

> {

>   int x1;

>   [e = n::make_shared (), x1]() {};

> }

>

> I.e., I don't think the fact that std::make_shared is a template plays a

> specific role here.

>

> Paolo.
diff --git gcc/cp/parser.c gcc/cp/parser.c
index bcee1214c2f..fc11f9126d3 100644
--- gcc/cp/parser.c
+++ gcc/cp/parser.c
@@ -10440,6 +10440,12 @@ cp_parser_lambda_introducer (cp_parser* parser, tree lambda_expr)
 		   capture_init_expr,
 		   /*by_reference_p=*/capture_kind == BY_REFERENCE,
 		   explicit_init_p);
+
+      /* If there is any qualification still in effect, clear it
+       * now; we will be starting fresh with the next capture.  */
+      parser->scope = NULL_TREE;
+      parser->qualifying_scope = NULL_TREE;
+      parser->object_scope = NULL_TREE;
     }
 
   cp_parser_require (parser, CPP_CLOSE_SQUARE, RT_CLOSE_SQUARE);
diff --git gcc/testsuite/g++.dg/cpp1y/pr71546.C gcc/testsuite/g++.dg/cpp1y/pr71546.C
new file mode 100644
index 00000000000..934a6b32364
--- /dev/null
+++ gcc/testsuite/g++.dg/cpp1y/pr71546.C
@@ -0,0 +1,11 @@
+// PR c++/71546
+// { dg-do compile { target c++14 } }
+// { dg-options "" }
+
+namespace n { struct make_shared { }; }
+
+int main()
+{
+  int x1;
+  [e = n::make_shared (), x1]() {};
+}
Jason Merrill Feb. 27, 2018, 8:29 p.m. | #4
On 02/27/2018 01:51 PM, Håkon Sandsmark wrote:
> Thanks for the feedback. I chose to take the example from the bug

> report verbatim as the test case.

> 

> However, I agree it makes sense to have the simplest possible test

> case that reproduces the issue. Here is an updated patch.


Thanks!

> +      /* If there is any qualification still in effect, clear it

> +       * now; we will be starting fresh with the next capture.  */


For future reference, we don't add * at the beginning of subsequent 
lines in a comment.  I'll correct that in this patch and check it in.

It looks like you don't have a copyright assignment on file with the FSF 
yet.  This patch is small enough not to need one, but if (as I hope) 
you're thinking to continue contributing to GCC, you might want to file 
an assignment for future changes.

Jason
Jason Merrill Feb. 27, 2018, 9:02 p.m. | #5
On 02/27/2018 03:29 PM, Jason Merrill wrote:
> On 02/27/2018 01:51 PM, Håkon Sandsmark wrote:

>> Thanks for the feedback. I chose to take the example from the bug

>> report verbatim as the test case.

>>

>> However, I agree it makes sense to have the simplest possible test

>> case that reproduces the issue. Here is an updated patch.

> 

> Thanks!

> 

>> +      /* If there is any qualification still in effect, clear it

>> +       * now; we will be starting fresh with the next capture.  */

> 

> For future reference, we don't add * at the beginning of subsequent 

> lines in a comment.  I'll correct that in this patch and check it in.


Done.  FYI I also renamed the testcase to lambda-init17.C; I sometimes 
like to run e.g. the *lambda* tests as a smoke test, and "pr12345" isn't 
very useful for that.

Jason
Håkon Sandsmark Feb. 27, 2018, 9:09 p.m. | #6
2018-02-27 22:02 GMT+01:00 Jason Merrill <jason@redhat.com>:
> On 02/27/2018 03:29 PM, Jason Merrill wrote:

>>

>> On 02/27/2018 01:51 PM, Håkon Sandsmark wrote:

>>>

>>> Thanks for the feedback. I chose to take the example from the bug

>>> report verbatim as the test case.

>>>

>>> However, I agree it makes sense to have the simplest possible test

>>> case that reproduces the issue. Here is an updated patch.

>>

>>

>> Thanks!

>>

>>> +      /* If there is any qualification still in effect, clear it

>>> +       * now; we will be starting fresh with the next capture.  */

>>

>>

>> For future reference, we don't add * at the beginning of subsequent lines

>> in a comment.  I'll correct that in this patch and check it in.

>

>

> Done.  FYI I also renamed the testcase to lambda-init17.C; I sometimes like

> to run e.g. the *lambda* tests as a smoke test, and "pr12345" isn't very

> useful for that.


Thanks for the quick turnaround! I was unsure about the naming of the
file myself.

I'll definitely look into the copyright assignment for the future.

> Jason


Håkon

Patch

diff --git gcc/cp/parser.c gcc/cp/parser.c
index bcee1214c2f..fc11f9126d3 100644
--- gcc/cp/parser.c
+++ gcc/cp/parser.c
@@ -10440,6 +10440,12 @@  cp_parser_lambda_introducer (cp_parser* parser, tree lambda_expr)
 		   capture_init_expr,
 		   /*by_reference_p=*/capture_kind == BY_REFERENCE,
 		   explicit_init_p);
+
+      /* If there is any qualification still in effect, clear it
+       * now; we will be starting fresh with the next capture.  */
+      parser->scope = NULL_TREE;
+      parser->qualifying_scope = NULL_TREE;
+      parser->object_scope = NULL_TREE;
     }
 
   cp_parser_require (parser, CPP_CLOSE_SQUARE, RT_CLOSE_SQUARE);
diff --git gcc/testsuite/g++.dg/cpp1y/pr71546.C gcc/testsuite/g++.dg/cpp1y/pr71546.C
new file mode 100644
index 00000000000..861563aacf9
--- /dev/null
+++ gcc/testsuite/g++.dg/cpp1y/pr71546.C
@@ -0,0 +1,11 @@ 
+// PR c++/71546
+// { dg-do compile { target c++14 } }
+// { dg-options "" }
+
+#include <memory>
+
+int main()
+{
+  int x1;
+  [e = std::make_shared <int> (), x1]() {};
+}