[cxx-interop] Fix two headers to be valid in C++ mode. #2895
Conversation
|
@swift-ci test |
|
@swift-ci please test. |
|
I don't have commit access. Could someone review/merge this for me? |
zoecarver
changed the title
Not urgent. I'm just eager to get that other patch in 😁 I had forgotten that toolchain PR testing exists, thanks for running that. Anyway, I'm fine waiting for one of the reviewers to merge. |
|
Friendly ping. |
|
@compnerd @millenomi would you have a moment to take a look, or would you mind if I merge this otherwise? |
|
Hmm, where is |
When C++ interop is enabled, any time these are used it's in a C++ context. The same is true for Swift shims (we had to make similar fixes there a while back). |
|
In other words, it's used in C++ mode any time a Swift program imports I'll update the commit messages to clarify that this is needed to import these into programs that use the |
This is exported as a C header but in C++ mode ucal.h will expose templates that aren't valid. When a program uses the flag "-enable-cxx-interop" this library will be imported in a C++ context so we can only have valid C++ code.
This C99 extensions is only valid in C. When a program uses the flag "-enable-cxx-interop" this library will be imported in a C++ context so we can only have valid C++ code.
zoecarver
force-pushed
the
fix/skip-icu-in-cxx-mode
branch
from
380e732 to
f82f4cb
Compare
This comment has been minimized.
This comment has been minimized.
|
I've updated the commit message. @compnerd any other comments? |
This comment has been minimized.
This comment has been minimized.
|
@swift-ci please test Linux platform. |
3 similar comments
|
@swift-ci please test Linux platform. |
|
@swift-ci please test Linux platform. |
|
@swift-ci please test Linux platform. |
|
Friendly ping. |
|
@swift-ci please smoke test Windows platform. |
|
@swift-ci please test Windows platform. |
|
@millenomi friendly ping. Any comments? |
|
I'm a little slow here but I'm not sure I get the change for _Internal, which should not be imported outside of the CF build itself, which is in C mode and not C++. Okay on the ForSwift… because that ends up in the CF module, tho. |
| @@ -19,7 +19,7 @@ | |||
| #include "CFDateComponents.h" | |||
| #include "CFDateInterval.h" | |||
|
|
|||
| #if __has_include(<unicode/ucal.h>) | |||
| #if __has_include(<unicode/ucal.h>) && !defined(__cplusplus) | |||
There was a problem hiding this comment.
Per comment: please explain here.
There was a problem hiding this comment.
With C++ Interoperability enabled, all headers are imported as C++ headers. So they will all be imported in C++ mode. Even if they're not publically exposed. In other words, all headers in the CF module will be imported as C++ headers in a program that enables C++ interoperability.
You're right, I think ForSwift may be the only place this is causing issues. To be honest, I know next to nothing about core foundation so, maybe this isn't the correct fix. Feel free to suggest an alternative.
Anyway, does that answer your question? Would you also like me to put a comment in "CFCalendar_Internal.h" explaining this?
There was a problem hiding this comment.
@millenomi friendly ping. I'd really like to get this moving.
There was a problem hiding this comment.
My bad, I missed your ping back.
There was a problem hiding this comment.
No worries. I missed your comment the first time ;) Thanks!
|
@swift-ci please test and merge |
|
In the interest of getting this going, I'm okay with the safety change on the internal header. |
Great. If you'd like to keep discussing, I'm happy to. (And if we arrive at a better solution, I'm happy to implement it.) |
This is exported as a C header but in C++ mode
ucal.hwill expose templates that aren't valid. This will hopefully fix the Linux build for swiftlang/swift#32721.