[cxx-interop] Add flag to set minimum access level for reverse interop #84816
Conversation
Xazax-hun requested review from egorzhdan, j-hui and zoecarver as code owners Xazax-hun requested review from artemcm, fahadnayyar, hnrklssn, susmonteiro and tshortli as code owners Xazax-hun force-pushed the reverse-interop-non-public branch from 8e4b387 to 8f0dc63 Compare 
test/Interop/SwiftToCxx/core/set-min-access-level-and-expose-explicitly.swift Outdated Show resolved Hide resolved
test/Interop/SwiftToCxx/core/set-min-access-level-and-expose-explicitly.swift Show resolved Hide resolved
Xazax-hun force-pushed the reverse-interop-non-public branch from 8f0dc63 to 96bf7db Compare 
| llvm::raw_string_ostream moduleContents{moduleContentsBuf}; | ||
| auto deps = printModuleContentsAsCxx( | ||
| moduleContents, *M, interopContext, | ||
| frontendOpts.ClangHeaderMinAccess.value_or(AccessLevel::Public), |
There was a problem hiding this comment.
We currently do not seem to use the same heuristics for C++ interop (we never call getRequiredAccess). I did not want to change that behavior in this PR so leaving this as is for now. Let me know if you disagree.
| os, *M->getASTContext().getStdlibModule(), interopContext, | ||
| /*requiresExposedAttribute=*/true, exposedModules); | ||
| printModuleContentsAsCxx(os, *M->getASTContext().getStdlibModule(), | ||
| interopContext, AccessLevel::Public, |
There was a problem hiding this comment.
Why is this fixed as AccessLevel::Public? (Quite possible there’s a legitimate reason; I’m just wondering what it is.)
There was a problem hiding this comment.
This call is used to provide C++ a way to access certain types in Swift's standard library and we do not want to expose the non-public parts of it.
| .Case("public", AccessLevel::Public) | ||
| .Case("package", AccessLevel::Package) | ||
| .Case("internal", AccessLevel::Internal) | ||
| .Default(std::nullopt); |
There was a problem hiding this comment.
Should there be an explicit spelling for the default behavior? Something like auto or default?
There was a problem hiding this comment.
I am a bit torn on this. Looking at the compiler invocation, seeing something like default would potentially mean that the behavior depends on the compiler version. I think that makes it a bit harder to understand what that invocation is doing and also the project more prone to break when/if the default change. Since I am split, I usually tend to lean towards providing less. Because it is always easier to add something in the future than taking it away. What do you think?
Xazax-hun force-pushed the reverse-interop-non-public branch from 96bf7db to 89869a4 Compare 
| @swift-ci please smoke test |
3 participants
rdar://159211965