review: feature sub inheritance hierarchy function

[pvojtechovsky]

The SubInheritanceHierarchyFunction is created and configured with one or more types. This mapping function returns all sub types of configured types, which are found in packages, which are input for this mapping function.
This mapping function is more efficient then filtering all types using 'SubtypeFilter', because:

• it allows searching for subtypes of more then one super type
• it visits each type of spoon model only once, during inheritance hierarchy checking.
• if new super type is added (configured), then next scanning always returns only newly found sub types

This new mapping function is used by #1291, which

• needs to scan sub inheritance hierarchy of more types in more iterations, where each iteration needs to scan similar sub hierarchy, but with some new types.
• each next iteration has to return only newly found subtypes
• it should be fast, because I need to refactor nearly all methods of model of my project.

[monperrus]

It seems that this function takes a type as input, and optionally a package. What about designing it the other way (CtConsumableFunction<CtTypeInformation>):

public class SubInheritanceHierarchyFunction implements CtConsumableFunction<CtTypeInformation>, CtQueryAware {
public SubInheritanceHierarchyFunction() // default to RootPackage
public SubInheritanceHierarchyFunction(CtPackage)
public void apply(CtTypeInformation input, final CtConsumer<Object> outputConsumer) 

The tests would be clearer this way.

[pvojtechovsky]

What about designing it the other way (CtConsumableFunction):

then it does not fit to my needs. But I agree that clients will think same like you, so what about:

1. to move code of SubInheritanceHierarchyFunction to different (more internal) package, to rename it to something like SubInheritanceHiearchyResolver and to not inherit from CtConsumableFunction ... because it's contract does not fits to contract of other mapping functions.
2. to create new simple mapping function, which will behave like you suggested and which will internally use new SubInheritanceHiearchyResolver

[monperrus]

so what about: 1. to move code of |SubInheritanceHierarchyFunction| to different (more internal) package, to rename it to something like |SubInheritanceHiearchyResolver| and to not inherit from CtConsumableFunction ... because it's contract does not fits to contract of other mapping functions. 2. to create new simple mapping function, which will behave like you suggested and which will internally use new |SubInheritanceHiearchyResolver|

perfect!

…

--Martin

[pvojtechovsky]

Move and rename done. Seems to be OK from my point of view. Please fix my documentation/English ;-) Thanks!

[pvojtechovsky]

Let's fix #1292 first. This PR depends on it, so refactoring will be easier without it.

[monperrus]

rebase?

[monperrus]

OK for the responsibility of this class. On the design, I'd suggest a stateless one (stateless is often better).

What about the following:

• REMOVE field targetSuperTypes, constructor SubInheritanceHierarchyResolver(CtTypeInformation), method addSuperType
• ADD a new parameter to method forEachSubTypeInPackage which represents a collection of CtTypeInformation

WDYT?

[pvojtechovsky]

interesting idea!
... but my new solution is kind of compromise ;-), because I need to call forEachSubTypeInPackage several times using same state, to search for next set of sub types after new super type(s) is/are added.
Anyway this solution is more understandable then origin one, because now it is clear whether client starts a new search or continues searching using existing state.

[monperrus]

private?

[pvojtechovsky]

thanks! I forgot to mark it private.

[pvojtechovsky]

Now the SubInheritanceHierarchyFunction is stateless. Is such solution acceptable?
It is ready for merge from my point of view... please check java doc as usually...

[monperrus]

Thanks Pavel for the changes. Here are a couple of changes to reduce the API surface and increase encapsulation by putting put a lot of things private. It still compiles and the tests run. Are you OK with this? Other question: most boolean flags in SubInheritanceHierarchyFunction and SubInheritanceHierarchyResolver are not tested. Do you need them all?

[pvojtechovsky]

private

These private methods make implementation of #1291 much harder and less efficient (worse performance). I need these methods there, so I do not like the idea to make them private. Please have a look at algorithm of that PR, so it is understandable what I am speaking about.

[pvojtechovsky]

Other question: most boolean flags in SubInheritanceHierarchyFunction and
SubInheritanceHierarchyResolver are not tested.
Do you need them all?

I do not need them now/yet, but they copy the pattern of other mapping functions. For example

• boolean includingSelf - is same in: CtScannerFunction, OverriddenMethodFilter, OverridingMethodFilter, ParentFunction, SiblingsFunction, SubtypeFilter
  other boolean switches are with similar pattern and makes the queries based on these functions more flexible/powerful.

[monperrus]

Hi Pavel,

On the private methods, I've privatized 4 of them. Do you need them all? Which ones do you really need?

On the boolean flags, I'm not against them, I only much prefer specified/tested code. AFAIU, most of those flags are untested.

[pvojtechovsky]

Hi Martin,

On the private methods, they were an implementation of the contract: to be able to search for other (not yet found) sub types of newly added super types. Note that java class hierarchies often share their supertypes so it makes sense to keep the results of previous sub type search and to search only for new sub types. Without these methods (which you made private) the algorithm has to scan whole model for sub types again and will return before found sub types (duplicities), so

1. it needs more computation
2. it needs more complex client code, because client has to filter out already found sub types.

May be the algorithm I am describing is specific for #1291 so the private methods might be switched to package protected ... but I do not vote for this solution.

Think about contract of SubInheritanceHierarchyFunction and SubInheritanceHiearchyResolver. What is the difference between them after you make these methods private? Nearly nothing. So if I would agree with private methods, then we might simplify it even more and keep only SubInheritanceHierarchyFunction ...

But then I miss main contract of SubInheritanceHiearchyResolver - to be able to search for extended set of sub types after new super type(s) is/are added. and I would be not able to implement #1291 efficiently

[pvojtechovsky]

On the boolean flags, I'm not against them, I only much prefer specified/tested code.

sure, I will add tests after we found a compromize between size of API and contracts of SubInheritanceHiearchyResolver.
I do not want to takeover your changes into my workspace, because it causes compilation problems in my code.

[monperrus]

I see. We have again a baby PR problem here because we have three things: - SubInheritanceHierarchyFunction - SubInheritanceHierarchyResolver and its core responsibility and contracts - the various boolean options As we're now used to, let's split the problem so as to facilitate convergence and merge. I propose to start with SubInheritanceHierarchyResolver (without any other class). Thanks to your explanation, I start to better understand what you mean, the responsibility and contract you need I'll isolate SubInheritanceHierarchyResolver in one separate PR, and try to test it to see whether it corresponds to your plan.

[monperrus]

removed my last commit

[pvojtechovsky]

Rebase done but, build server has problem:

Error while storing the mojo status: No space left on device

[surli]

Rebase done but, build server has problem

I just had the same issue, it should be ok now. You can do a minor change or a an empty commit to trigger the CI.

[monperrus]

thanks!