[NFC] Adopt TypeBase-isms for GenericSignature #27436
Conversation
|
apple/swift-lldb#2022 |
| @@ -388,7 +388,8 @@ ValueDecl *RequirementFailure::getDeclRef() const { | |||
| do { | |||
| if (auto *parent = DC->getAsDecl()) { | |||
| if (auto *GC = parent->getAsGenericContext()) { | |||
| if (GC->getGenericSignature() != Signature) | |||
| // FIXME: Is this intending an exact match? | |||
| if (GC->getGenericSignature().getPointer() != Signature.getPointer()) | |||
There was a problem hiding this comment.
@xedin Should this canonicalize the generic signatures, or are we looking for an exact match here?
|
Nitpick: because there are no subclasses, can you call it GenericSignatureImpl or signature? |
|
Alternately, the indirect one could be GenericSignatureRef, like ProtocolConformanceRef. |
|
I lean towards |
Structurally prevent a number of common anti-patterns involving generic signatures by separating the interface into GenericSignature and the implementation into GenericSignatureBase. In particular, this allows the comparison operators to be deleted which forces callers to canonicalize the signature or ask to compare pointers explicitly.
CodaFi
force-pushed
the
signed-sealed-delivered
branch
from
c9f850a to
5a8d074
Compare
|
@swift-ci please smoke test |
|
apple/swift-lldb#2022 |
|
⛵️ |
| @@ -541,7 +541,7 @@ class alignas(1 << TypeAlignInBits) TypeBase { | |||
|
|
|||
| /// allowsOwnership() - Are variables of this type permitted to have | |||
| /// ownership attributes? | |||
| bool allowsOwnership(GenericSignature *sig=nullptr); | |||
| bool allowsOwnership(GenericSignatureImpl *sig = nullptr); | |||
There was a problem hiding this comment.
Why isn't this a GenericSignature?
| @@ -4376,15 +4377,15 @@ ASTContext::getOverrideGenericSignature(const ValueDecl *base, | |||
| return nullptr; | |||
| } | |||
|
|
|||
| if (derivedClass->getGenericSignature() == nullptr && | |||
| if (derivedClass->getGenericSignature().isNull() && | |||
There was a problem hiding this comment.
Is there no implicit conversion to bool?
| ctx.evaluator, | ||
| AbstractGenericSignatureRequest{ | ||
| derivedClass->getGenericSignature(), | ||
| derivedClass->getGenericSignature().getPointer(), |
There was a problem hiding this comment.
This unwrapping should not be necessary
| @@ -446,7 +446,7 @@ Type ASTBuilder::createImplFunctionType( | |||
| ArrayRef<Demangle::ImplFunctionResult<Type>> results, | |||
| Optional<Demangle::ImplFunctionResult<Type>> errorResult, | |||
| ImplFunctionTypeFlags flags) { | |||
| GenericSignature *genericSig = nullptr; | |||
| GenericSignature genericSig = GenericSignature(); | |||
There was a problem hiding this comment.
It should be default initialized
| @@ -215,7 +215,7 @@ GenericEnvironment::mapConformanceRefIntoContext( | |||
| ProtocolConformanceRef conformance) const { | |||
| auto contextConformance = conformance.subst(conformingInterfaceType, | |||
| QueryInterfaceTypeSubstitutions(this), | |||
| LookUpConformanceInSignature(*getGenericSignature())); | |||
| LookUpConformanceInSignature(getGenericSignature().getPointer())); | |||
There was a problem hiding this comment.
More interfaces to clean up
|
I think we can go further here and make GenericSignature like SubstitutionMap. Instead of forwarding method calls from GenericSignature to GenericSignatureImpl with operator-> we can implement the methods on GenericSignature itself. This way we can also implement meaningful behavior for the empty value -- just treat it as a signature with no parameters and no requirements. This would in turn simplify away lots of null checks all over the place. |
Structurally prevent a number of common anti-patterns involving generic
signatures by separating the interface into GenericSignature and the
implementation into GenericSignatureBase. In particular, this allows
the comparison operators to be deleted which forces callers to
canonicalize the signature or ask to compare pointers explicitly.