-
Notifications
You must be signed in to change notification settings - Fork 10.5k
GenericSpecializer: don't pre-specialize C++ reference type for AnyObject #83090
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
Conversation
@swift-ci test |
@swift-ci apple silicon benchmark |
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.
Thank you! Just one comment on the tests.
|
||
@inline(never) | ||
func go() { | ||
let x: RefType = RefType.makeRefType() |
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.
Minor: can we test this in test/Interop/Cxx/foreign-reference/reference-counted.swift
? It already has some testing for arrays of foreign reference types (ReferenceCountedTestSuite.test("Global array")
).
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.
As discussed offline, I moved the test to test/Interop/Cxx/foreign-reference
d3ad3fa
to
40e0360
Compare
@swift-ci test |
lib/SILOptimizer/Utils/Generics.cpp
Outdated
@@ -3163,7 +3163,8 @@ static bool usePrespecialized( | |||
|
|||
if (lowered.isBuiltinBridgeObject() && layout->isBridgeObject()) { | |||
newSubs.push_back(genericParam->getASTContext().TheBridgeObjectType); | |||
} else if (lowered.hasRetainablePointerRepresentation()) { | |||
} else if (lowered.hasRetainablePointerRepresentation() && |
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.
Doesn’t this mean that hasRetainablePointerRepresentation() should be changed? I’m pretty sure other callers also assume AnyObject compatibility when this returns true.
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'm not sure. hasRetainablePointerRepresentation
is used in the type checker but arc for imported c++ classes is mostly about code generation...
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.
@slavapestov Actually, I agree. It's safer to change hasRetainablePointerRepresentation
. I also added a comment.
@swift-ci smoke test macos |
…ject Reference counting is not compatible. Fixes a runtime crash if a C++ reference type is put into an Array. swiftlang#83082 rdar://155919841
40e0360
to
f962d76
Compare
@swift-ci smoke test |
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.
Thanks!
Reference counting is not compatible.
Fixes a runtime crash if a C++ reference type is put into an Array.
#83082
rdar://155919841