-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Swift: Improve getABaseType implementions #14252
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
…lType (inconsistent, difficult to find) to Type.
…e them and (2) consistently. This is not behaviour preserving.
MathiasVP
left a comment
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 this is a good direction to move in (and hence my approval). I'll wait with clicking merge to see if there are anyone else who disagree with this change.
|
Thanks for the review. I agree about giving others chance to comment. DCA shows a substantial analysis performance regression (14.9%), definitely worth looking into... |
|
... I was able to identify |
|
DCA shows a reduced (7.3%) analysis performance regression now. Stage timings now blames |
|
The repeat DCA run shows a different spike ( |
|
@redsun82 are you happy with the changes here / direction for |
redsun82
left a comment
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 definitely agree with you about type aliases, and the changes LGTM.
|
Great, lets merge this! 🎉 |
Improve the predicates
getABaseType,getABaseTypeDecl,getADerivedTypeandgetADerivedTypeDeclonTypeandTypeDecl:Bto the underlyingAinclass B : A_aliasTypeAlias.getABaseType()has no result now. Previously there was special code to make this work forType(but notTypeDecl). See below*for more discussion of this point.TypeDecl.qlladded by @redsun82 in Swift: renamebase_typesinTypeDecltoinherited_types#14208 is resolved.*- regarding the old special case forTypeAlias.getABaseType(). We could restore the old behaviour (and extend it toTypeDecl), but I currently don't think it's the right thing to do and I want to discuss that (and perhaps be persuaded to change my mind). Consider:As this PR stands now,
getABaseTypetakes us fromBtoAandgetADerivedTypefromAtoB. Simple. It's the users job to call.getUnderlyingTypeif they might be working with aB_alias.If we have
getABaseTypetake us fromB_aliastoAas well (that is from bothBandB_aliastoA), then (assuming we keep the symmetry between them...)getADerivedTypehas two results forA, one of which is itself an alias. But then surely we wantgetADerivedTypeto work in the other direction fromA_alias- implying additional results fromgetABaseTypeas well. That means both functions are many-to-many, and hard to think clearly about. Especially if there are more type aliases than in the example above...As this PR stands I have removed the special case, updated the existing code that relied on it, and called it out explicitly in the change note.
There's a good chance this change will affect results on tests I haven't run locally. Once CI passes, I will do a DCA run as well.