Clean method removal of RPackage #14360
Conversation
This is a first step at the cleaning of the method removal of RPackage. This is still quite bad but it is a first step a cleaning this. The change make the method removal work as the method addition and reduces the usage of the cache variables of RPackage a little. My mid term goal is to kill some of those caches.
jecisc
added
the
Status: Need more work
jecisc
added
Status: Tests passed please review!
and removed
Status: Need more work
| ifTrue: [ | ||
| metaclassDefinedSelectors at: methodClass instanceSide originalName ifPresent: [ :methods | | ||
| methods remove: aCompiledMethod selector ifAbsent: [ ]. | ||
| methods ifEmpty: [ metaclassDefinedSelectors removeKey: methodClass instanceSide originalName ] ] ] | ||
| ifFalse: [ | ||
| classDefinedSelectors at: methodClass originalName ifPresent: [ :methods | | ||
| methods remove: aCompiledMethod selector ifAbsent: [ ]. | ||
| methods ifEmpty: [ classDefinedSelectors removeKey: methodClass originalName ] ] ] ] |
There was a problem hiding this comment.
On add is used name as key, but here is using originalName. What's the difference?
There was a problem hiding this comment.
The difference happens with ObsoleteClasses. #name will return the obsolete name while #originalName will return the name of the class when it was not obsolete.
In the end I would like to have real classes instead of class names in the caches so that we do not have to care about that (and so that we do not have to cast them each time we want a class)
| methodClass isMeta | ||
| ifTrue: [ | ||
| metaclassExtensionSelectors at: methodClass instanceSide originalName ifPresent: [ :methods | | ||
| methods remove: aCompiledMethod selector ifAbsent: [ ]. | ||
| methods ifEmpty: [ metaclassExtensionSelectors removeKey: methodClass instanceSide originalName ] ] ] | ||
| ifFalse: [ | ||
| classExtensionSelectors at: methodClass originalName ifPresent: [ :methods | | ||
| methods remove: aCompiledMethod selector ifAbsent: [ ]. | ||
| methods ifEmpty: [ classExtensionSelectors removeKey: methodClass originalName ] ] ] ]. |
There was a problem hiding this comment.
Should be nice to hide and reuse the use of the cache in both methods.
Maybe delegating to methods
self cacheFor: methodClass ifPresent: [ .. ]
self cacheFor: methodClass ifAbsent: [ .. ]
There was a problem hiding this comment.
I'm planning to kill some of those caches. For now I'm trying to make the code simpler so that I can do my changes.
I also want to merge the class and metaclass caches because it's a bother to always check the #isMeta to call two different methods. Once this is done, I'll see if we can improve further the use of the extension selectors :)
This beast is pretty huge and I'm trying to simplify little by little because each change has a lot of impacts ahah
| methods ifEmpty: [ classExtensionSelectors removeKey: methodClass originalName ] ] ] ]. | ||
|
|
||
| ((metaclassExtensionSelectors at: methodClass instanceSide originalName ifAbsent: [ #( ) ]) isEmpty and: [ | ||
| (classExtensionSelectors at: methodClass instanceSide originalName ifAbsent: [ #( ) ]) isEmpty ]) ifTrue: [ |
There was a problem hiding this comment.
classExtensionSelectors is using methodClass originalName (or name) as key.
Copy-paste issue?
There was a problem hiding this comment.
There is no issue because here we are in the #ifFalse: of methodClass isMeta so we know we already have the instance side.
But as I said in another comment, I would like to remove the meta/non meta management to use one unified way.
This is a first step at the cleaning of the method removal of RPackage.
This is still quite bad but it is a first step a cleaning this. The change make the method removal work as the method addition and reduces the usage of the cache variables of RPackage a little. My mid term goal is to kill some of those caches.