[interop] update names and add docs for the interop C++ helper macros #64911
Conversation
hyp
force-pushed
the
eng/grand-macro-rename
branch
from
69a9149 to
5c03b36
Compare
|
@swift-ci please test |
|
@swift-ci please test source compatibility |
|
@swift-ci please test macOS platform |
|
@swift-ci please test source compatibility |
|
@swift-ci please test macOS platform |
hyp
force-pushed
the
eng/grand-macro-rename
branch
from
5c03b36 to
30d0c17
Compare
|
@swift-ci please test |
|
Hmm, did you consider prefixing them with a bunch of __underscores, to avoid collisions with existing user code? I could totally see it both ways. Like, NS_RETURNS_RETAINED isn't prefixed with __underscores, so maybe it's fine. But given that you're building a language feature that could be useful outside of Apple's constrained ecosystem, it might make sense to keep these macros in language-feature-y style. While extremely unlikely, it's theoretically possible that even the C++ standard itself could want to reserve some of these identifiers in the future, so it could get really awkward. |
|
I also really like it that these names aren't alluding to Swift! These attributes tell us things about the C++ classes regardless of Swift interop, they can be useful for other applications, such as static analysis. I still think there could be a catchy prefix for these attributes all together, so that the reader could clearly see that they're part of the same facility and didn't need to look up each one separately, in order to learn that when they're reading the code. But it looks like other compiler features (eg. https://clang.llvm.org/docs/ThreadSafetyAnalysis.html) didn't go down that route; so maybe my intuition isn't quite correct here. |
|
When we used such attrs elsewhere we prefixed with SWIFT_ I guess, so that might be a possibility? |
For now we thought that we would rather avoid SWIFT_ prefix. |
|
@swift-ci please test |
|
@swift-ci please test source compatibility |
|
@swift-ci please build toolchain |
|
@swift-ci please test macOS platform |
|
@swift-ci please build toolchain |
|
@swift-ci please test |
|
@swift-ci please test source compatibility |
| @@ -30,15 +30,15 @@ import Test | |||
| // CHECK: note: record 'A' is not automatically available: does not have a copy constructor or destructor. Does this type have reference semantics? | |||
| // CHECK: struct A { | |||
| // CHECK: ^ | |||
| // CHECK: SWIFT_REFERENCE_TYPE(<#retain#>, <#release#>) | |||
| // CHECK: SHARED_REFERENCE(<#retain#>, <#release#>) | |||
There was a problem hiding this comment.
I think these need SWIFT_
| NOTE(mark_safe_to_import, none, "Mark method '%0' as 'SAFE_TO_IMPORT' in C++ to " | ||
| "make it available in Swift. ", | ||
| NOTE(mark_safe_to_import, none, "annotate method '%0' with 'SWIFT_RETURNS_INDEPENDENT_VALUE' in C++ to " | ||
| "make it available in Swift", | ||
| (StringRef)) | ||
|
|
||
| NOTE(at_to_subscript, none, "Do you want to replace it with a call " |
There was a problem hiding this comment.
Non blocking, but if it's easy, there's a reference to "import_reference" in this file that should probably be updated. Not a big deal, though.
|
Still LGTM once tests are passing :) |
hyp
force-pushed
the
eng/grand-macro-rename
branch
from
7b7062f to
64a4b31
Compare
|
@swift-ci please test |
|
@swift-ci please test source compatibility |
|
source compat failures are unrelated. |
No description provided.