Skip to content

[cxx-interop] Do not emit IR for C++20 requires expr #65652

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

Merged
merged 1 commit into from
May 5, 2023

Conversation

egorzhdan
Copy link
Contributor

This fixes a compiler crash when emitting IR for a for-in loop over a C++ std::vector in C++20 mode.

rdar://108810356

@egorzhdan egorzhdan added the c++ interop Feature: Interoperability with C++ label May 4, 2023
@egorzhdan egorzhdan requested a review from ravikandhadai May 4, 2023 14:07
@egorzhdan egorzhdan requested review from zoecarver and hyp as code owners May 4, 2023 14:07
@egorzhdan
Copy link
Contributor Author

@swift-ci please smoke test

@egorzhdan
Copy link
Contributor Author

@swift-ci please test

Copy link
Contributor

@hyp hyp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Beautiful, thank you!

@@ -119,6 +119,10 @@ class ClangDeclFinder
return true;
}

bool TraverseRequiresExpr(clang::RequiresExpr *RE) {
return false;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually this should be true. if you return false the traversal exits early which will make us miss some symbols. Can you test that in your test? e.g. make your function call something else and make sure that's emitted

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, thanks for noticing this!

Copy link
Contributor

@hyp hyp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please update traversal logic

@egorzhdan egorzhdan force-pushed the egorzhdan/cxx-requires-irgen branch from 8959f9f to 0974fca Compare May 4, 2023 14:30
@egorzhdan
Copy link
Contributor Author

@swift-ci please smoke test

@egorzhdan
Copy link
Contributor Author

@swift-ci please test

@egorzhdan egorzhdan requested a review from hyp May 4, 2023 14:30
@egorzhdan egorzhdan force-pushed the egorzhdan/cxx-requires-irgen branch from 0974fca to 95da9ea Compare May 4, 2023 16:19
@egorzhdan
Copy link
Contributor Author

@swift-ci please smoke test

This fixes a compiler crash when emitting IR for a for-in loop over a C++ `std::vector` in C++20 mode.

rdar://108810356
@egorzhdan egorzhdan force-pushed the egorzhdan/cxx-requires-irgen branch from 95da9ea to 21745e5 Compare May 5, 2023 16:21
@egorzhdan
Copy link
Contributor Author

@swift-ci please smoke test

@@ -0,0 +1,10 @@
// RUN: %target-swiftxx-frontend -emit-ir -Xcc -std=gnu++20 -I %S/Inputs %s | %FileCheck %s
//
// REQUIRES: OS=macosx
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a note saying why this needs macOS in C++ 20 mode . Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Filed #65710

@egorzhdan egorzhdan merged commit 9a496e8 into main May 5, 2023
@egorzhdan egorzhdan deleted the egorzhdan/cxx-requires-irgen branch May 5, 2023 18:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ interop Feature: Interoperability with C++
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants