[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
j-hui
left a comment
j-hui
left a comment
There was a problem hiding this comment.
LGTM, aside from one suggestion:
Can you add a test checking that the filter works for non-top-level items? Eg a struct with a mix of public and internal methods and fields.
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
beccadax
left a comment
beccadax
left a comment
There was a problem hiding this comment.
This is a really nice enhancement, but I’d like it to work differently for C and ObjC interop.
Xazax-hun
force-pushed
the
reverse-interop-non-public
branch
from
96bf7db to
89869a4
Compare
|
@swift-ci please smoke test |
| static AccessLevel getRequiredAccess(const ModuleDecl &M) { | ||
| static AccessLevel getRequiredAccess(const ModuleDecl &M, | ||
| std::optional<AccessLevel> minAccess) { | ||
| if (minAccess) |
There was a problem hiding this comment.
I should have brought this up last time, but since you're writing this in a way that affects ObjC generated headers, would you be willing to add test cases for that?
I took a look and I think you'd get adequate coverage by just adding another set of RUN lines to test/PrintAsObjC/accessibility.swift:
// RUN: %target-swift-frontend(mock-sdk: %clang-importer-sdk) -parse-as-library %s -application-extension-library -emit-clang-header-min-access internal -typecheck -verify -emit-objc-header-path %t/accessibility-explicit-internal.h -disable-objc-attr-requires-foundation-module
// RUN: %FileCheck -check-prefix=CHECK -check-prefix=CHECK-INTERNAL %s < %t/accessibility-explicit-internal.h
// RUN: %check-in-clang %t/accessibility-explicit-internal.h
// RUN: %target-swift-frontend(mock-sdk: %clang-importer-sdk) -parse-as-library %s -application-extension -emit-clang-header-min-access public -typecheck -verify -emit-objc-header-path %t/accessibility-explicit-public.h -disable-objc-attr-requires-foundation-module
// RUN: %FileCheck -check-prefix=CHECK -check-prefix=CHECK-PUBLIC %s < %t/accessibility-explicit-public.h
// RUN: %check-in-clang %t/accessibility-explicit-public.hThis test is already set up to exercise all the heuristics; we're just adding another pair of cases that override the heuristics with your new flag.
Assuming that passes, that's the only issue I see at this point—feel free to merge once it's resolved.
There was a problem hiding this comment.
I added these tests and they pass. Thanks a lot! Will merge this PR, but I am also happy to address if anything comes up post-merge.
Xazax-hun
force-pushed
the
reverse-interop-non-public
branch
from
89869a4 to
adca01b
Compare
|
@swift-ci please smoke test |
This is a counterpart to swiftlang/swift#84816. rdar://159211965
This is a counterpart to swiftlang/swift#84816. rdar://159211965
3 participants
rdar://159211965