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

add simple support for enter/exit in DJPP as in CtScanner #1538

Merged
merged 4 commits into from
Sep 19, 2017

Conversation

monperrus
Copy link
Collaborator

design exploration for #1494

@pvojtechovsky
Copy link
Collaborator

Why? To use CtScanner#enter and #exit methods for listening? Or do you mean something else?

@monperrus
Copy link
Collaborator Author

monperrus commented Sep 18, 2017 via email

@pvojtechovsky
Copy link
Collaborator

I did not debugged it, but looking at code, it looks like CtScanner#enter/exit are NEVER called by DJPP, because it always directly calls DJPP#scan method.

@monperrus
Copy link
Collaborator Author

Good point! See c256281

My point here is not to duplicate existing responsibilities... and thanks to this discussion, we can even make DJPP come under CtScanner.

@pvojtechovsky
Copy link
Collaborator

pvojtechovsky commented Sep 18, 2017

add call to enter and exit

Then, I suggest to keep DJPP extends CtVisitor and just add new
protected enter/exit(CtElement){} methods into DJPP, same like they are in CtScanner.

I have feeling like the extending from CtScanner is confusing, because all the code of CtScanner is never called.

But new enter/exit methods of DJPP, might be good substitution for my PrinterListener. I can accept that solution for now.

@monperrus
Copy link
Collaborator Author

I have feeling like the extending from CtScanner is confusing, because all the code of CtScanner is never called.

I tend to agree...

958c353 is an alternative. WDYT?

@pvojtechovsky
Copy link
Collaborator

depends on CtAbstractVisitor instead

is better then CtScanner.

Do we want to force change of DJPP if we add new method into CtVisitor?
If DJPP extends CtAbstractVisitor, then new CtVisitor method is added to CtAbstractVisitor and may stay empty in DJPP, so DJPP will do nothing for that new method.
Actually all the methods of DJPP implements some behavior for each method of CtVisitor. So it would make sense to force it by DJPP implements CtVisitor.

But I can live with both solutions. It is up to You ;-)

@monperrus
Copy link
Collaborator Author

monperrus commented Sep 18, 2017

good point again. KISSed it accordingly: cc72383

@monperrus monperrus changed the title what if DJPP extends CtScanner? add simple support for enter/exit in DJPP as in CtScanner Sep 18, 2017
@INRIA INRIA deleted a comment from spoon-bot Sep 18, 2017
@pvojtechovsky
Copy link
Collaborator

Looks good for me. Even the scan(CtReference ref) seems to be correct way. If tests pass, I agree with merge.

@spoon-bot
Copy link
Collaborator

Revapi Analysis results

Old API: fr.inria.gforge.spoon:spoon-core:jar:6.0.0-20170918.191525-22

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

Detected changes: 1.

Change 1

Name Element
Old method spoon.reflect.visitor.DefaultJavaPrettyPrinter spoon.reflect.visitor.DefaultJavaPrettyPrinter::scan(spoon.reflect.reference.CtReference)
New none
Code java.method.removed
Description Method was removed.
Breaking binary: breaking

@INRIA INRIA deleted a comment from spoon-bot Sep 19, 2017
@INRIA INRIA deleted a comment from spoon-bot Sep 19, 2017
@INRIA INRIA deleted a comment from spoon-bot Sep 19, 2017
@INRIA INRIA deleted a comment from spoon-bot Sep 19, 2017
@pvojtechovsky pvojtechovsky merged commit 494ba6f into INRIA:master Sep 19, 2017
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.

3 participants