-
Notifications
You must be signed in to change notification settings - Fork 0
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
Split class details into separate object. #224
Conversation
Right now, details like supertypes and properties are all included in the `ClassTypeEmbedding`. This is unfortunate, because these are data classes, and this breaks the equality definition on them. With this commit, all the mutable data is split off into a separate object. The class type embeddings still aren't quite interchangable, since some may be missing this `details` object, but at least it's much easier to detect now.
@@ -38,6 +38,7 @@ class ProgramConverter(val session: FirSession, override val config: PluginConfi | |||
ProgramConversionContext { | |||
private val methods: MutableMap<MangledName, FunctionEmbedding> = SpecialKotlinFunctions.byName.toMutableMap() | |||
private val classes: MutableMap<MangledName, ClassTypeEmbedding> = mutableMapOf() | |||
private val classDetails: MutableMap<ClassTypeEmbedding, ClassEmbeddingDetails> = mutableMapOf() |
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 actually need classDetails
when we have details
property in every ClassTypeEmbedding
?
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 guess there're issues with the initialization order, but I don't quite get them.
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.
The relevant situation here is when you get a class embedding but don't have associated details. I guess in that case we could trust that the copy in classes
does have an embedding, though... I'll think about it.
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 ended up removing this map, thanks for the suggestion :)
} | ||
else -> existingEmbedding | ||
val embedding = classes.getOrPut(className) { | ||
ClassTypeEmbedding(className, symbol.classKind == ClassKind.INTERFACE) | ||
} |
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.
Intuitively isInterface
should be included in details
. Did you leave it as is because the situation is much simpler in this case?
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, that's a fair point. I left it because it was in the constructor arguments in ClassTypeEmbedding
, but that's not really a good way to do it.
hierarchyPath?.forEach { classType -> | ||
val predAcc = classType.sharedPredicateAccessInvariant().fillHole(receiverWrapper) | ||
val predAcc = classType.details.sharedPredicateAccessInvariant().fillHole(receiverWrapper) |
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.
Hmm, I think .details
can be omitted here since you've overridden this method anyway.
In general, should we hide those .details
calls under some extension functions of TypeEmbedding
?
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 TypeEmbedding
is the best place; I'd prefer us to fail explicitly when we need a ClassTypeEmbedding
but have something else. We could make functions that forward things to details, but I'm not sure there's much benefit; details
is not meant to be private, and being aware you're using the details can be seen as a plus.
Right now, details like supertypes and properties are all included in the `ClassTypeEmbedding`. This is unfortunate, because these are data classes, and this breaks the equality definition on them. With this commit, all the mutable data is split off into a separate object. The class type embeddings still aren't quite interchangable, since some may be missing this `details` object, but at least it's much easier to detect now.
Right now, details like supertypes and properties are all included in the `ClassTypeEmbedding`. This is unfortunate, because these are data classes, and this breaks the equality definition on them. With this commit, all the mutable data is split off into a separate object. The class type embeddings still aren't quite interchangable, since some may be missing this `details` object, but at least it's much easier to detect now.
Right now, details like supertypes and properties are all included in the
ClassTypeEmbedding
. This is unfortunate, because these are data classes, and this breaks the equality definition on them. With this commit, all the mutable data is split off into a separate object. The class type embeddings still aren't quite interchangable, since some may be missing thisdetails
object, but at least it's much easier to detect now.