-
Notifications
You must be signed in to change notification settings - Fork 12.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Clang][Sema] Tweak tryCaptureVariable for unevaluated lambdas #93206
Merged
+116
−11
Merged
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
658e9d4
[Clang][Sema] Tweak tryCaptureVariable for unevaluated lambdas
zyn0217 60aff76
Cleanup duplicate tests
zyn0217 36d6277
Remove dead codes in ActOnLambdaExpressionAfterIntroducer
zyn0217 dc2519b
Merge branch 'main' into try-capture-variable
zyn0217 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any good reason to do this in 2 places, rather than just delay the above one to here? @cor3ntin ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, looking into
ActOnLambdaClosureQualifiers
, I don't see anything that (currently) depends on lambda parameters. I guess we can combine these two calls? I'd love to open a separate NFC PR for doing so, before landing this patch. :)@erichkeane I retrospected your comment in #78598 (comment) and I appreciate the diagnostics for non-ODR uses - however that's much like a QoI issue to me and would it be OK to leave it as a follow-up? Let me know what you think and I'll add a FIXME then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I'd be ok with this as a followup. But @cor3ntin is the one who knows the most about lambda stuff (particularly parsing, etc), so I'd like to see him approve this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we need to set whether the lambda has a
mutable
keyword as soon as the parameters, if present, are parsed, as that affect the meaning of subsequent id-expressionsWhen there are no parens, there can be a noexcept clause for example, and parsing that does requires knowing whether the lambda is mutable, so that we can adjust the type of any reference-to-capture.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, that makes sense! We currently set
LSI->Mutable
inActOnLambdaClosureQualifiers()
, and while I think it's possible to move it out of the function, I'm not opposed to leaving it as-is.