[review needed] speed up RBClassNotReferencedRule
Hi, Review needed for https://pharo.fogbugz.com/f/cases/16074/Is-RBClassNotReferencedRule-200x-slo... Speedup can be checked with: rule:=RBClassNotReferencedRule new. environment:=RBBrowserEnvironment new forPackageNames: #('Fuel'). [RBSmalllintChecker runRule: rule onEnvironment: environment] timeToRun Pharo5: "0:00:00:13.526" Pharo5 with fix: "0:00:00:00.761" Pharo3: 0:00:00:00.547 Checking all classes for usage: [Smalltalk allClassesAndTraits select: [ : each | each isUsed not]] timeToRun still takes 1:40 to find 600 unused classes. This shows that adding a cache for âreferenced classesâ per environment would speed this up further. But it should be general, not just for RB or even RB Code Critique. The nice things is that code environments only change when you add classes or methods, so adding a very general cache should be quite possible. Another thing we should add is a way for classes to define âI am usedâ. E.g this now finds all Configurations, all the Manifests and all cases like the RB Rules themselves. All these should know that they are not useless even though there are no references. Marcus
Marcus Denker-4 wrote
Hi,
Review needed for
https://pharo.fogbugz.com/f/cases/16074/Is-RBClassNotReferencedRule-200x-slo...
Speedup can be checked with:
rule:=RBClassNotReferencedRule new. environment:=RBBrowserEnvironment new forPackageNames: #('Fuel'). [RBSmalllintChecker runRule: rule onEnvironment: environment] timeToRun
Pharo5: "0:00:00:13.526" Pharo5 with fix: "0:00:00:00.761" Pharo3: 0:00:00:00.547
Checking all classes for usage:
[Smalltalk allClassesAndTraits select: [ : each | each isUsed not]] timeToRun
still takes 1:40 to find 600 unused classes.
This shows that adding a cache for âreferenced classesâ per environment would speed this up further. But it should be general, not just for RB or even RB Code Critique.
The nice things is that code environments only change when you add classes or methods, so adding a very general cache should be quite possible.
Another thing we should add is a way for classes to define âI am usedâ. E.g this now finds all Configurations, all the Manifests and all cases like the RB Rules themselves. All these should know that they are not useless even though there are no references.
Marcus
Thanks for working on this. I have a use for it this week. For methods RB allows one to add a pragma e.g.: <lint: 'Methods implemented but not sent' rationale: 'this code plays a role in examining the system' author: 'pad'> and described here: http://www.lukas-renggli.ch/blog/ignoring-lint-rules. Maybe we can hijack that mechanism with a class side method e.g. rbNotReferenced <lint> Do your modifications test sends of subclassesDo: and friends? OR #(#MyClass1 #MyClass2 #MyClass3) collect:[:ea | (Smalltalk at: ea) doSomething]. The reason I ask is I'm planning to do a bunch of cleanup of a package and it would be nice to not worry about those cases getting missed -- View this message in context: http://forum.world.st/review-needed-speed-up-RBClassNotReferencedRule-tp4845... Sent from the Pharo Smalltalk Developers mailing list archive at Nabble.com.
Maybe we can hijack that mechanism with a class side method e.g.
rbNotReferenced <lint>
Do your modifications test sends of subclassesDo: and friends?
OR
#(#MyClass1 #MyClass2 #MyClass3) collect:[:ea | (Smalltalk at: ea) doSomething].
The reason I ask is I'm planning to do a bunch of cleanup of a package and it would be nice to not worry about those cases getting missed
What I did is to implement #isUsed like this in e.g the hierarchy of RBLintRule: isUsed "all my sublasses are used" ^self name ~= âRBLintRule' this way if RBLintRule is not used, it is seen by the Critique, but all subclasses are used by default. I implemented that in TestCase, too, and removed the explicit check for TestCase subclasses from the rule. Marcus
On 18 Sep 2015, at 08:34, Marcus Denker <marcus.denker@inria.fr> wrote:
Maybe we can hijack that mechanism with a class side method e.g.
rbNotReferenced <lint>
Do your modifications test sends of subclassesDo: and friends?
OR
#(#MyClass1 #MyClass2 #MyClass3) collect:[:ea | (Smalltalk at: ea) doSomething].
The reason I ask is I'm planning to do a bunch of cleanup of a package and it would be nice to not worry about those cases getting missed
What I did is to implement #isUsed like this in e.g the hierarchy of RBLintRule:
isUsed "all my sublasses are used" ^self name ~= âRBLintRule'
this way if RBLintRule is not used, it is seen by the Critique, but all subclasses are used by default.
I implemented that in TestCase, too, and removed the explicit check for TestCase subclasses from the rule.
This is a nice thing. In the same way we can deal with abstract classes. So if someone develops an abstract class with an intent to use it, he can specify that itâs ok to use the class. Also is there any work on pragmas for classes, at some point Iâve heard something about it but canât recall. I will try to hack a bit this weekend and see if I can add some kind of automation to marking a class as used. I.e. if you see a critic, you should be able to mark the class as being used with 1-2 clicks. If you are developing some king of facade, you should know that you will not reference it from your library. Uko
Marcus
What I did is to implement #isUsed like this in e.g the hierarchy of RBLintRule:
isUsed "all my sublasses are used" ^self name ~= âRBLintRule'
this way if RBLintRule is not used, it is seen by the Critique, but all subclasses are used by default.
I implemented that in TestCase, too, and removed the explicit check for TestCase subclasses from the rule.
This is a nice thing. In the same way we can deal with abstract classes. So if someone develops an abstract class with an intent to use it, he can specify that itâs ok to use the class.
True.
Also is there any work on pragmas for classes, at some point Iâve heard something about it but canât recall.
Yes, the question is where to put it syntactically and how to save it in monticello⦠could be interesting to have.
I will try to hack a bit this weekend and see if I can add some kind of automation to marking a class as used. I.e. if you see a critic, you should be able to mark the class as being used with 1-2 clicks. If you are developing some king of facade, you should know that you will not reference it from your library.
ok. Marcus
On Fri, 2015-09-18 at 14:48 +0200, Marcus Denker wrote:
What I did is to implement #isUsed like this in e.g the hierarchy of RBLintRule:
isUsed "all my sublasses are used" ^self name ~= âRBLintRule'
this way if RBLintRule is not used, it is seen by the Critique, but all subclasses are used by default.
I implemented that in TestCase, too, and removed the explicit check for TestCase subclasses from the rule.
This is a nice thing. In the same way we can deal with abstract classes. So if someone develops an abstract class with an intent to use it, he can specify that itâs ok to use the class.
True.
Also is there any work on pragmas for classes, at some point Iâve heard something about it but canât recall.
Yes, the question is where to put it syntactically
After a class declaration like this: LLVMDisposableObject subclass:#LLVMDIBuilder instanceVariableNames:'' classVariableNames:'' poolDictionaries:'LLVMModuleFlagBehavior' category:'LLVM-S-Core' Annotation key: 'disposable:' value: 'true' This ANSI-compatible way (see page 39 of NCITS J20 DRAFT of ANSI Smalltalk Standard, rev 1.9). Perhaps nice way is Annotation disposable: true
and how to save it in monticelloâ¦
In the source as above. In the binary, in a separate entry in the .zip so the other MC implementation won't crash but silently ignore them.
Yes, the question is where to put it syntactically
After a class declaration like this:
LLVMDisposableObject subclass:#LLVMDIBuilder instanceVariableNames:'' classVariableNames:'' poolDictionaries:'LLVMModuleFlagBehavior' category:'LLVM-S-Core'
Annotation key: 'disposable:' value: 'true'
This ANSI-compatible way (see page 39 of NCITS J20 DRAFT of ANSI Smalltalk Standard, rev 1.9).
Perhaps nice way is
Annotation disposable: true
and how to save it in monticelloâ¦
In the source as above. In the binary, in a separate entry in the .zip so the other MC implementation won't crash but silently ignore them.
Yes, that looks good. I made a note (no idea when I will come around implementing it, though). Marcus
Sorry guys but this is terrible because it is implicitly scoped. From an analysis point of view we have to rely on order to
After a class declaration like this:
LLVMDisposableObject subclass:#LLVMDIBuilder instanceVariableNames:'' classVariableNames:'' poolDictionaries:'LLVMModuleFlagBehavior' category:'LLVM-S-Core'
Annotation key: 'disposable:' value: 'true'
This ANSI-compatible way (see page 39 of NCITS J20 DRAFT of ANSI Smalltalk Standard, rev 1.9).
Perhaps nice way is
Annotation disposable: true A better way would be
LLVMDIBuilder disposable: true or LLVMDIBuilder annotationKey: #disposable: value: true
2015-08-26 10:25 GMT+02:00 Marcus Denker <marcus.denker@inria.fr>:
Hi,
Review needed for
https://pharo.fogbugz.com/f/cases/16074/Is-RBClassNotReferencedRule-200x-slo...
Speedup can be checked with:
rule:=RBClassNotReferencedRule new. environment:=RBBrowserEnvironment new forPackageNames: #('Fuel'). [RBSmalllintChecker runRule: rule onEnvironment: environment] timeToRun
Pharo5: "0:00:00:13.526" Pharo5 with fix: "0:00:00:00.761" Pharo3: 0:00:00:00.547
Checking all classes for usage:
[Smalltalk allClassesAndTraits select: [ : each | each isUsed not]] timeToRun
still takes 1:40 to find 600 unused classes.
This shows that adding a cache for âreferenced classesâ per environment would speed this up further. But it should be general, not just for RB or even RB Code Critique.
The nice things is that code environments only change when you add classes or methods, so adding a very general cache should be quite possible.
Another thing we should add is a way for classes to define âI am usedâ. E.g this now finds all Configurations, all the Manifests and all cases like the RB Rules themselves. All these should know that they are not useless even though there are no references.
Marcus
But now, we have the problem that this rule does not find references to classes by symbol. The comment for this rule says: "This smell arises when a class is not referenced either directly or indirectly by a symbol." But all spec adapter classes are only referenced by their symbol and this now isn't found anymore: MorphicMenuGroupAdapter isUsed " - > false "
participants (6)
-
Jan Vrany -
Marcus Denker -
Nicolai Hess -
Paul DeBruicker -
stepharo -
Yuriy Tymchuk