Skip to content
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

review: feature: ParentFunction #1153

Merged
merged 4 commits into from
Feb 1, 2017
Merged

Conversation

pvojtechovsky
Copy link
Collaborator

Implements new mapping functions, which visits all the parents of current element.
It is needed by to LocalVariableScopeFunction to detect scope of local variable, by searching for a parent which matches some rules... will be used in next PR, which fixes LocalVariableScopeFunction for variables used in CtLoop expressions and CtTryWithResource.

Note: I have one question to the scanning of all parents. It is "Which parent is the top parent?"
If I visit parents until the getParent()==null, then I get to helper annonymous class based on CtElementImpl, which is declared in CtModelImpl$CtRootPackage$1. I guess it is not correct ... Your opinion please?
The test actually fails because the scanning does not end on RootPackage, but one level below...

@surli
Copy link
Collaborator

surli commented Jan 31, 2017

If I visit parents until the getParent()==null,

why don't you use getParent() == factory.getModel().getRootPackage() as condition to stop?

@pvojtechovsky
Copy link
Collaborator Author

I know several ways how to do it different/better. But I did not know what is the correct behavior from the spoon design point of view. So I waited for a feedback of an experienced spooner.
So, please confirm that parent visiting should stop on factory.getModel().getRootPackage() - including that node. And I will do so. Or discuss it and let me know what is correct. Thank You!

@tdurieux
Copy link
Collaborator

Note: I have one question to the scanning of all parents. It is "Which parent is the top parent?"
If I visit parents until the getParent()==null, then I get to helper annonymous class based on CtElementImpl, which is declared in CtModelImpl$CtRootPackage$1. I guess it is not correct ... Your opinion please?
The test actually fails because the scanning does not end on RootPackage, but one level below...

If I correctly remember, we only created the CtRootPackage to "store" all elements of a model. It does not represent any Java concept.
It is probably better to include CtRootPackage in order to match the behavior of getParent(Filter<E> filter).
Even though there is no interest to visit it.

@msteinbeck
Copy link
Contributor

If I correctly remember, we only created the CtRootPackage to "store" all elements of a model. It does not represent any Java concept.

Isn't it the package of files without any package declaration?

@tdurieux
Copy link
Collaborator

Isn't it the package of files without any package declaration?

Yes, and also for the parent of declared packages.
Before CtRootPackage, we identified the default package by comparing its name to TOP_LEVEL_PACKAGE_NAME

@pvojtechovsky
Copy link
Collaborator Author

Thanks to everybody for good feedback. I modified the code and test so it expects that last visited parent element is CtModelImpl$CtRootPackage.

@spoon-bot
Copy link
Collaborator

Revapi Analysis results

Old API: fr.inria.gforge.spoon:spoon-core:jar:5.6.0-20170130.234600-20

New API: fr.inria.gforge.spoon:spoon-core:jar:5.6.0-SNAPSHOT

Detected changes: 0.

@surli
Copy link
Collaborator

surli commented Feb 1, 2017

Ok it seems good to me.

@surli surli merged commit e514e17 into INRIA:master Feb 1, 2017
@spoon-bot
Copy link
Collaborator

Revapi Analysis results

Old API: fr.inria.gforge.spoon:spoon-core:jar:5.6.0-20170131.234706-21

New API: fr.inria.gforge.spoon:spoon-core:jar:5.6.0-SNAPSHOT

Detected changes: 0.

@surli
Copy link
Collaborator

surli commented Feb 1, 2017

Test revapi.

@pvojtechovsky pvojtechovsky deleted the ParentFunction branch February 2, 2017 19:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants