[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
| } | ||
|
|
||
| @_expose(Cxx) | ||
| private func privateFunc(_ x: Int) -> Int { |
There was a problem hiding this comment.
Since this is never emitted, should we warn about sub-internal decls being annotated?
There was a problem hiding this comment.
Maybe. This is also used for wasm and that is a bit special. I will need to double check with someone to see it is OK to rule that out.
Xazax-hun
force-pushed
the
reverse-interop-non-public
branch
from
8f0dc63 to
96bf7db
Compare
|
@swift-ci please smoke test |
| static AccessLevel getRequiredAccess(const ModuleDecl &M, | ||
| AccessLevel minAccess) { | ||
| return M.isExternallyConsumed() ? minAccess : AccessLevel::Internal; |
There was a problem hiding this comment.
I don’t think I like this behavior. If you explicitly specify an access level on the command line, that ought to override the implicit computation completely, even when isExternallyConsumed() is false. In other words, I’d like to see:
| static AccessLevel getRequiredAccess(const ModuleDecl &M, | |
| AccessLevel minAccess) { | |
| return M.isExternallyConsumed() ? minAccess : AccessLevel::Internal; | |
| static AccessLevel getRequiredAccess(const ModuleDecl &M, | |
| std::optional<AccessLevel> minAccess) { | |
| if (minAccess) | |
| return *minAccess; | |
| return M.isExternallyConsumed() ? AccessLevel::Public : AccessLevel::Internal; |
With matching changes upstream of this function to preserve the optionality of ClangHeaderMinAccess up to this point.
| printModuleContentsAsC(cModuleContents, imports, *M, interopContext); | ||
| printModuleContentsAsC( | ||
| cModuleContents, imports, *M, interopContext, | ||
| frontendOpts.ClangHeaderMinAccess.value_or(AccessLevel::Public)); |
There was a problem hiding this comment.
The value_or here would need to be removed for my suggestion above.
| printModuleContentsAsObjC(objcModuleContents, imports, *M, interopContext); | ||
| printModuleContentsAsObjC( | ||
| objcModuleContents, imports, *M, interopContext, | ||
| frontendOpts.ClangHeaderMinAccess.value_or(AccessLevel::Public)); |
| llvm::raw_string_ostream moduleContents{moduleContentsBuf}; | ||
| auto deps = printModuleContentsAsCxx( | ||
| moduleContents, *M, interopContext, | ||
| frontendOpts.ClangHeaderMinAccess.value_or(AccessLevel::Public), |
| 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.)
| .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?
3 participants
rdar://159211965