Summary: | offer modern apis to work on AST | ||
---|---|---|---|
Product: | [Eclipse Project] JDT | Reporter: | Carsten Hammer <carsten.hammer> |
Component: | UI | Assignee: | JDT-UI-Inbox <jdt-ui-inbox> |
Status: | NEW --- | QA Contact: | |
Severity: | enhancement | ||
Priority: | P3 | CC: | jjohnstn |
Version: | 4.21 | ||
Target Milestone: | --- | ||
Hardware: | All | ||
OS: | All | ||
See Also: |
https://git.eclipse.org/r/c/jdt/eclipse.jdt.ui/+/184558 https://bugs.eclipse.org/bugs/show_bug.cgi?id=421101 |
||
Whiteboard: |
Description
Carsten Hammer
2021-08-28 08:45:58 EDT
I created some small samples to compare the current way to work with ASTVisitor and a possible additional implementation on top of it providing new capabilities. see here https://github.com/carstenartur/sandbox/wiki/HelperVisitor-sample https://github.com/carstenartur/sandbox/wiki/HelperVisitor-more-simple-samples https://github.com/carstenartur/sandbox/wiki/HelperVisitor-more-complex-samples https://github.com/carstenartur/sandbox/wiki/ASTProcessor-samples For the simple examples I provided sample code based on ASTVisitor to compare it but for the more complex examples it is not always possible. (In reply to Carsten Hammer from comment #1) > I created some small samples to compare the current way to work with > ASTVisitor and a possible additional implementation on top of it providing > new capabilities. > > see here > > https://github.com/carstenartur/sandbox/wiki/HelperVisitor-sample > > https://github.com/carstenartur/sandbox/wiki/HelperVisitor-more-simple- > samples > > https://github.com/carstenartur/sandbox/wiki/HelperVisitor-more-complex- > samples > > https://github.com/carstenartur/sandbox/wiki/ASTProcessor-samples > > For the simple examples I provided sample code based on ASTVisitor to > compare it but for the more complex examples it is not always possible. Thanks Carsten. Hi Carsten, I am wondering if the design could be greatly simplified if the ReferenceHolder is always String -> Object. With this, there is no need for type parameters and anything is still basically possible. For example, the reference holder could have special get/set methods for a nodes processed Set using a special reserved String. The get code could allocate one if one doesn't already exist. This again simplifies what the end-user needs to specify in constructors, etc... It also makes all visitor methods reusable. A default ReferenceHolder could then be created by the HelperVisitor and there could be getReferenceHolder(), setReferenceHolder() calls in addition to two constructors (one taking a ReferenceHolder, the other one creating one on behalf of the user). I also think that the HelperVisitor should incorporate AST in its name as it is used for processing ASTNodes. Perhaps, ASTNodeVisitorHelper. I also noticed that the HelpVisitor predicatedata map is only allowing one item. So, for a method invocation, I can only select one method name. It would be useful to allow a list of names. Again, perhaps the reference holder can store this information: "filteredMethodInvocationNames" -> List<String> and this could be wrapped in getFilteredMethodInvocationNames()/setFilteredMethodInvocationNames(). You have an example using BiPredicate.or() but I think this doesn't work the way you want it to. Normal visitor methods return true to indicate keep looking down in children, but the or() method short-circuits so the after call won't occur if the first call returns true. I believe and() would work for the example. (In reply to Jeff Johnston from comment #3) > Hi Carsten, > > I am wondering if the design could be greatly simplified if the > ReferenceHolder is always String -> Object. With this, there is no need for > type parameters and anything is still basically possible. For example, the > reference holder could have special get/set methods for a nodes processed > Set using a special reserved String. The get code could allocate one if one > doesn't already exist. This again simplifies what the end-user needs to > specify in constructors, etc... It also makes all visitor methods reusable. Yes, maybe it is better always to have the map as start and then allow to put data in it. My intention starting with specific types using generics was to allow a completely type safe design right from the start even for apis designed on top of it and allow to change to a different map that allows to work in parallel in different threads on the same ast if needed (making use of eg. concurrent maps). Maybe it is not a blocking point to loose type safety even if it hurts :) > > A default ReferenceHolder could then be created by the HelperVisitor and > there could be getReferenceHolder(), setReferenceHolder() calls in addition > to two constructors (one taking a ReferenceHolder, the other one creating > one on behalf of the user). Yes, if going that way such a change is a good idea. > > I also think that the HelperVisitor should incorporate AST in its name as it > is used for processing ASTNodes. Perhaps, ASTNodeVisitorHelper. Yes. Names are the most difficult thing :) > > I also noticed that the HelpVisitor predicatedata map is only allowing one > item. So, for a method invocation, I can only select one method name. It > would be useful to allow a list of names. Again, perhaps the reference > holder can store this information: "filteredMethodInvocationNames" -> > List<String> and this could be wrapped in > getFilteredMethodInvocationNames()/setFilteredMethodInvocationNames(). Yes, I can think of many more searches searching for nodes where you can express what you are looking for in a few method parameters instead. Again I would like to be as type safe as possible. Maybe full type safety is only possible with one type per node type. > > You have an example using BiPredicate.or() but I think this doesn't work the > way you want it to. Normal visitor methods return true to indicate keep > looking down in children, but the or() method short-circuits so the after > call won't occur if the first call returns true. I believe and() would work > for the example. Could be that needs some more changes… Thanks for having a look! |