Bugzilla – Full Text Bug Listing
| Summary: | [DOM] Move NodeFinder to a non-internal package | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Eclipse Project] JDT | Reporter: | Martin Kersten <Martin.Kersten> | ||||||||||
| Component: | Core | Assignee: | Olivier Thomann <Olivier_Thomann> | ||||||||||
| Status: | VERIFIED FIXED | QA Contact: | |||||||||||
| Severity: | enhancement | ||||||||||||
| Priority: | P3 | CC: | caniszczyk, daniel_megert, david_williams, jerome_lanneluc, Konstantin.Scheglov, markus.kell.r, naci.dai, rodrigo, stephan.herrmann | ||||||||||
| Version: | 3.0 | ||||||||||||
| Target Milestone: | 3.5 M3 | ||||||||||||
| Hardware: | All | ||||||||||||
| OS: | All | ||||||||||||
| Whiteboard: | |||||||||||||
| Bug Depends on: | |||||||||||||
| Bug Blocks: | 292345, 250944 | ||||||||||||
| Attachments: |
|
||||||||||||
|
Description
Martin Kersten
Also the GenericVisitor would be a nice to have feature. Maybe the ASTNodeVisitor may replicate the behaviour. But may be you need it for other purpose. Don't know. But I think the AST API still lacks ways to find a particular node. Would be nice to have the NodeFinder and GenericVisitor in a non-internal package. Deferred I would like to repoen this bug for comments: It would be nice to make NodeFinder or at least GenericVisitor API. WTP EJB code generation makes use of them (for a simple task to find ASTNodes of TypeDecleration) I think they are useful outside the boundaries of JST Adding david to CC Olivier, please look at this for M3 Need to synchronize with JDT/UI since they could then use the core version of the class. Right now this class is inside the JDT/UI component. Markus, can we see this together post M2 ? Created attachment 112408 [details] add boolean ASTVisitor#preVisit2(ASTNode) > Markus, can we see this together post M2 ? Sure, but unfortunately I'll be away till 2008-10-06, so I can only give feedback in 3 weeks. NodeFinder needs some Javadoc cleanup, and I would - make it final or @noextend, and - use a private class to implement the actual ASTVisitor I think the GenericVisitor is only there because of a deficiency in the ASTVisitor API. The problem is that the generic preVisit(ASTNode) should return a boolean like all the type-specific visit(*) methods. See the attached patch for how to make the GenericVistor obsolete. Created attachment 115071 [details]
Proposed fix
This patch overrides the previous one by defining a new NodeFinder class inside the org.eclipse.jdt.core.dom package.
It might evolve over time to meet more specific requirements. (In reply to comment #7) > The problem is that the generic preVisit(ASTNode) should return > a boolean like all the type-specific visit(*) methods. Hmm, 'boolean preVisit2(ASTNode)' is enough for NodeFinder, but it's not a full replacement for the GenericVisitor. We have cases where subclasses of GenericVisitor override visitNode(ASTNode) as well as some visit(...) methods. We have to see case-by case whether preVisit2(ASTNode) is actually sufficient to implement all these cases, but I guess it's not. I don't have time to do this analysis for M3, so I opened bug 250944. We will probably still need GenericVisitor, either as a separate class, or integrated into the ASTVisitor. So Markus, should I release the actual patch and let you adapt to it to see if you need more changes ? > So Markus, should I release the actual patch and let you adapt to it to see if
> you need more changes ?
Yes, and if you want to be nice to other clients, you could add a warning that the API is still in flux to NodeFinder and ASTVisitor#preVisit2(ASTNode).
So I could add something like: This API is still subject to modifications before the 3.5 release. > This API is still subject to modifications before the 3.5 release. Yeah, or: This API is still under discussion, see https://bugs.eclipse.org/53024 . Created attachment 115150 [details]
Proposed fix
New patch with the warning in NodeFinder and ASTVisitor.preVisit2.
Released for 3.5M3. These is nothing to verify as is. Once JDT/UI will adopt this new API, all existing refactoring tests will extensively use it. Verified for 3.5M3 using JDT/Core plug-in source code of I20081026-2000. (In reply to comment #15) > Created an attachment (id=115150) [details] > Proposed fix > > New patch with the warning in NodeFinder and ASTVisitor.preVisit2. The warning is still in the sources. Since the bug is closed, should the comments be removed now? >Since the bug is closed, should the comments be removed now?
Yes. Olivier please commit my patch.
Created attachment 148752 [details]
Fix that removes outdated comments.
(In reply to comment #19) > Yes. Olivier please commit my patch. Done. *** Bug 220185 has been marked as a duplicate of this bug. *** |