This is an archive of the discontinued LLVM Phabricator instance.

[C++17] Reject shadowing of capture by parameter in lambda
ClosedPublic

Authored by Rakete1111 on Oct 23 2018, 11:32 AM.

Details

Summary

This change rejects the shadowing of a capture by a parameter in lambdas in C++17.

int main() {
  int a;
  auto f = [a](int a) { return a; };
}

results in:

main.cpp:3:20: error: a lambda parameter cannot shadow an explicitly captured entity
  auto f = [a](int a) { return a; };
                   ^
main.cpp:3:13: note: variable a is explicitly captured here
  auto f = [a](int a) { return a; };
            ^

Diff Detail

Repository
rC Clang

Event Timeline

Rakete1111 created this revision.Oct 23 2018, 11:32 AM

Thanks for working on this! Can you update www/cxx_dr_status.html too?

lib/Sema/SemaLambda.cpp
507

We usually back-port defect report resolutions to older standards, in this case maybe we should just warn pre-17 though. (Even though we warn here anyways with -Wshadow in -Wextra)

509

Nit: the colon should have a space on it's left here.

510

You should compare IdentifierInfos here, rather than StringRefs.

Rakete1111 added inline comments.Oct 23 2018, 12:31 PM
lib/Sema/SemaLambda.cpp
510

IdentifierInfo doesn't have a compare equal or operator== function. How do you propose I do that? Do I add one?

lib/Sema/SemaLambda.cpp
510

Oh sorry, I should have been more clear. An IdentifierInfo pointer should be the same for all instances of that identifier, so I just mean to compare the pointers like: Capture.Id == Param->getIdentifier().

Addressed review comments! :) Thanks

Rakete1111 marked 5 inline comments as done.Oct 23 2018, 1:53 PM

Use correct clang and version spelling.

lebedev.ri added inline comments.
include/clang/Sema/Sema.h
5584

Maybe this should be ArrayRef<LambdaIntroducer::LambdaCapture> Captures instead?

rsmith added inline comments.Oct 23 2018, 3:21 PM
include/clang/Basic/DiagnosticSemaKinds.td
6636–6638

Please just unconditionally reject this. We don't need to support this as an extension (right?) and our policy is that DRs apply retroactively (unless we have some good reason for them to not do so).

www/cxx_dr_status.html
13084

This file is autogenerated, please do not edit it manually.

Instead, you should add a suitable test in test/CXX/drs, with a // dr2211: 8 comment to indicate that this DR is addressed in Clang 8, and rerun the makx_cxx_dr_status script to regenerate this file. (You'll need to download a recent cwg_index.html file; the script uses it as input.)

For what it's worth, I think the language rule here is wrong, and we should instead be injecting the simple-captures into the lambda's function scope: http://lists.isocpp.org/core/2018/10/5145.php
But this appears to be a correct implementation of the rule as written, so let's go with it for now.

Addresed review comments :)

I updated the dr status file but a lot of unrelated changes made it in. Is this okay?

Rakete1111 marked 3 inline comments as done.Oct 24 2018, 1:45 AM
rsmith accepted this revision.Oct 25 2018, 12:12 PM

Addresed review comments :)

I updated the dr status file but a lot of unrelated changes made it in. Is this okay?

Please regenerate the file without your change and check that in as a separate commit prior to this.

This revision is now accepted and ready to land.Oct 25 2018, 12:12 PM

Also please mark PR39428 as fixed once this is submitted :)

Addresed review comments :)

I updated the dr status file but a lot of unrelated changes made it in. Is this okay?

Please regenerate the file without your change and check that in as a separate commit prior to this.

Why not afterwards? That way the DR is marked as resolved immediately.

Addresed review comments :)

I updated the dr status file but a lot of unrelated changes made it in. Is this okay?

Please regenerate the file without your change and check that in as a separate commit prior to this.

Why not afterwards? That way the DR is marked as resolved immediately.

That'd mean you need to track down an old version of the cwg_index.html file, though -- or guess what the relevant change to the HTML file would have been. Seems like more work that way. But the order of the two commits doesn't matter to me -- all I'm looking for is that the commit that resolves the DR /only/ changes that entry in cxx_dr_status.html, and the "rebase" on a new version of the issues list is done in a separate commit. (I don't know what you mean by "immediately"; I would expect the two commits to land within minutes of each other.)

Update DR list.

This revision was automatically updated to reflect the committed changes.