-
Notifications
You must be signed in to change notification settings - Fork 10.6k
[cxx-interop] SWIFT_NONCOPYABLE overlooked in some cases #84517
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[cxx-interop] SWIFT_NONCOPYABLE overlooked in some cases #84517
Conversation
|
@swift-ci please smoke test |
0c9e28f to
a1fe79a
Compare
|
@swift-ci please smoke test |
| @@ -68,6 +68,18 @@ MyPair<int, NonCopyableRequires> p7(); | |||
|
|
|||
| #endif | |||
|
|
|||
| template<typename T> | |||
| struct SWIFT_COPYABLE_IF(T) SWIFT_NONCOPYABLE DoubleAnnotation {}; | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to diagnose this? Or do we expect users to find themselves in a situation where this is useful?
Also could you add a test that checks that SWIFT_COPYABLE_IF(garbage) SWIFT_NONCOPYABLE DoubleAnnotation still diagnoses the invalid attribute?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this will be useful, but it's also not problematic that the user adds both. If we diagnose this, we should also diagnose the case where there's two or more SWIFT_COPYABLE_IF annotations in the same type (we currently merge all the parameters of these annotation) or when the user adds two or more SWIFT_NONCOPYABLE.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regarding the test you suggested, in the current implementation if there's a SWIFT_NONCOPYABLE we never even look at SWIFT_COPYABLE_IF, so we wouldn't diagnose the invalid attribute. I'm positive the same happens with escapable annotations: if a SWIFT_ESCAPABLE or SWIFT_NONESCAPABLE is present, we never parse the SWIFT_ESCAPABLE_IF
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should diagnose swift attributes that are obviously not used as intended.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure! I opened an issue: #84559
a1fe79a to
8bf955f
Compare
|
@swift-ci please smoke test |
|
@swift-ci please smoke test macos |
8bf955f to
cf7d394
Compare
|
@swift-ci please smoke test |
cf7d394 to
59f09c7
Compare
|
@swift-ci please smoke test |
59f09c7 to
7e14a8e
Compare
|
@swift-ci please smoke test |
In some cases, the use of
SWIFT_NONCOPYABLEwas being overlooked.Also, if this attribute is present, there's no need to evaluate the conditional parameters. We currently accept that the user adds both
SWIFT_NONCOPYABLEandSWIFT_COPYABLE_IFattributes to the same record, but if the first is present, we don't need to evaluate the second one.