Baby PR: SubInheritanceHierarchyResolver #1309
Conversation
monperrus
changed the title
|
@pvojtechovsky WDYT? |
monperrus
force-pushed
the
feat_SubInheritanceHierarchyResolver
branch
from
1931b3a to
c9530f1
Compare
|
Martin, thanks for Your effort. It looks quite good. Such implementation probably fits to needs of #1291 too. So I like it! I have only minor notes. I will add them into code |
| * Send them to outputConsumer and add then as targetSuperTypes too, to perform faster with detection of next sub types. | ||
| */ | ||
| if (!targetSuperTypes.contains(typeRef.getQualifiedName())) { | ||
| targetSuperTypes.add(typeRef.getQualifiedName()); |
There was a problem hiding this comment.
if (!targetSuperTypes.contains(typeRef.getQualifiedName())) {
targetSuperTypes.add(typeRef.getQualifiedName());can be replaced by
if (targetSuperTypes.add(typeRef.getQualifiedName())) {You have seen that in my origin code, and you changed it, so you probably think it as not understandable. So I at least suggest
String qName = typeRef.getQualifiedName();
if (!targetSuperTypes.contains(qName)) {
targetSuperTypes.add(qName);... spoon is already not fastest so why to wast time again?
I have feeling (not measured) like this code is in middle of often repeated algorithm. I plan to refactor 10 000 methods of 6000 classes and I need to search many many inheritance hiearchies of all these classes for all methods ... it can end with a big number of calls ;-)
| private boolean failOnClassNotFound = false; | ||
|
|
||
| public SubInheritanceHierarchyResolver(CtPackage input) { | ||
| inputPackage = input; |
There was a problem hiding this comment.
What about no package at all?
The default (root) inputPackage might be taken from the factory of CtTypeInformation delivered by first call of #addSuperType.
And if somebody ever needs to search in different then root package, then we can add #setStartPackage(CtPackage) method.
That setStartPackage is better then constructor, because it can be used to recursively scan on different packages... But I personally do not need that, so I vote for removing constructor parameter
There was a problem hiding this comment.
The default (root) inputPackage might be taken from the factory of CtTypeInformation delivered by first call of #addSuperType.
This would be implicit and not regular.
then we can add #setStartPackage(CtPackage) method.
setter = state = mutability = bad. the current design is append-only which is much better.
|
That's all. Thanks! |
yes! maintainability always comes first :-) optimization is just fine, but should come with very long and detailed comments to still be readable and maintainable. |
|
OK. Let's wait for @surli to review and merge this one, probably tomorrow. |
|
This PR is OK for me. Thanks! |
|
I'm reading the code, it seems OK but I have some questions as I'm still not really familiar with all changes introduced by Pavel :) |
| .setListener(new CtScannerListener() { | ||
| @Override | ||
| public ScanningMode enter(CtElement element) { | ||
| final CtTypeReference<?> typeRef = (CtTypeReference<?>) element; |
There was a problem hiding this comment.
Just to be sure: this cast here is always right because of the typeFilter and classFilter you're using above which guarantee you'll obtain CtTypeReference, right?
There was a problem hiding this comment.
This case is always right because SuperInheritanceHierarchyFunction by default always returns TypeReferences. My origin code declared it explicitly on line 145 where was
//internally it works always with references
.returnTypeReferences(true)but Martin cleared that ;-)
| * FOUND! we are in super inheritance hierarchy, which extends from an searched super type(s). | ||
| * All `currentSubTypes` are sub types of searched super type | ||
| */ | ||
| while (currentSubTypes.size() > 0) { |
There was a problem hiding this comment.
In fact, this part remains unclear to me: as far as I understand, you assume that the elements are returned with the following order: first the leaves, and then the superclasses/interfaces, right? This is a contract of the SuperInheritanceHierarchyFunction? Because I'm not sure to understand that when I'm reading its documentation.
There was a problem hiding this comment.
first the leaves, and then the superclasses/interfaces, right?
right. it is the contract of SuperInheritanceHierarchyFunction. It should be written in javadoc of SuperInheritanceHierarchyFunction class.
|
Ok then thanks @pvojtechovsky for the explanations :) |
baby PR for #1290