Page MenuHomePhabricator

[clang] fix linkage of nested lambda
ClosedPublic

Authored by blastrock on Jan 30 2020, 6:15 AM.

Details

Summary

This is an attempt to fix https://bugs.llvm.org/show_bug.cgi?id=44368

This effectively reverts D1783. It doesn't break the current tests and fixes the test that this commit adds.

We now decide of a lambda linkage only depending on the visibility of its parent context.

Diff Detail

Event Timeline

blastrock created this revision.Jan 30 2020, 6:15 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 30 2020, 6:15 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

+ rsmith

clang/lib/AST/Decl.cpp
1416–1417

The code is still inconsistent with the comment. Record is the lambda itself instead of the parent of the lambda.

This is effectively reverting https://reviews.llvm.org/D1783. If we don't need that any more for whatever reason, can we also get rid of the check for whether the outermost lambda has a mangling number a few lines above? (Please also check if we can remove getOutermostEnclosingLambda, which was added for this purpose.)

clang/lib/AST/Decl.cpp
1416–1417

Record->getDeclContext() returns the parent of the lambda.

blastrock updated this revision to Diff 241713.Jan 31 2020, 6:22 AM
blastrock edited the summary of this revision. (Show Details)

Thank you for the answers.

I have removed all the outermost lambda code and tests still pass.

hliao accepted this revision.Jan 31 2020, 7:26 AM

LGTM, but need @rsmith for the final review

This revision is now accepted and ready to land.Jan 31 2020, 7:26 AM
rsmith accepted this revision.Feb 4 2020, 6:41 PM

Thanks for the reviews! I do not have commit access, so please merge this whenever you can.

This revision was automatically updated to reflect the committed changes.
hliao added a comment.Feb 7 2020, 10:30 AM

Thanks for the reviews! I do not have commit access, so please merge this whenever you can.

done in 2926917f430. thanks for the patch!