Bugzilla – Full Text Bug Listing
| Summary: | MethodDefinitionInsertLocationFinder2 does not find implementation file when no definition already present | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | [Tools] CDT | Reporter: | Marc-André Laperle <malaperle> | ||||||
| Component: | cdt-refactoring | Assignee: | Sergey Prigogin <eclipse.sprigogin> | ||||||
| Status: | RESOLVED FIXED | QA Contact: | Emanuel Graf <emanuel> | ||||||
| Severity: | normal | ||||||||
| Priority: | P3 | CC: | cdtdoug, eclipse.sprigogin | ||||||
| Version: | 8.0 | ||||||||
| Target Milestone: | 8.0 | ||||||||
| Hardware: | All | ||||||||
| OS: | All | ||||||||
| Whiteboard: | |||||||||
| Bug Depends on: | |||||||||
| Bug Blocks: | 292851 | ||||||||
| Attachments: |
|
||||||||
Hi Sergey, this is the last (major?) issue that I'm aware of with the new Generate Getters and Setters. Few issues:
1. MethodDefinitionInsertLocationFinder2.find calls ToggleSourceAndHeaderUtil.getPartnerTranslationUnit, which in turn calls ASTProvider.runOnAST. This is bad since RefactoringASTCache is not being used.
2. ToggleSourceAndHeaderUtil.getPartnerFileFromFilename contains the following code:
if (tu.getResource() != null) {
partnerFile= findInContainer(tu.getResource().getParent(), partnerFileBasename);
}
if (partnerFile == null) {
partnerFile= findInContainer(tu.getCProject().getProject(), partnerFileBasename);
}
Instead of jumping directly from the parent folder to the project it would make sense to search in grand parent, grand grand parent, etc. folders first. This would take care of the following use case:
my project
component_a
public_headers
my_file.h
implementation
my_file.cc
component_b
public_headers
my_file.h
implementation
my_file.cc
Given component_b/public_headers/my_file.h we want to find component_b/implementation/my_file.cc, not component_a/implementation/my_file.cc.
3. In MethodDefinitionInsertLocationFinder2.find it's preferable to write insertLocation.getTranslationUnit() == null rather than insertLocation.getFile() == null
Created attachment 189252 [details] Improve search of implementation file (In reply to comment #2) > Few issues: > > 1. MethodDefinitionInsertLocationFinder2.find calls > ToggleSourceAndHeaderUtil.getPartnerTranslationUnit, which in turn calls > ASTProvider.runOnAST. This is bad since RefactoringASTCache is not being used. I extracted PartnerFileVisitor from PartnerFileComputer and created a second getPartnerTranslationUnit which uses RefactoringASTCache instead of ASTProvider.runOnAST. > 2. ToggleSourceAndHeaderUtil.getPartnerFileFromFilename contains the following > code: ... > Instead of jumping directly from the parent folder to the project it would make > sense to search in grand parent, grand grand parent, etc. folders first. Changed that to a while loop. It won't search at a IWorkspace level; I think this is preferable. I added a test for this and BaseTestFramework can now create missing folders as it imports files. > 3. In MethodDefinitionInsertLocationFinder2.find it's preferable to write > insertLocation.getTranslationUnit() == null rather than > insertLocation.getFile() == null Done! Fixed in HEAD > 20110219. *** cdt cvs genie on behalf of sprigogin *** Bug 337040 - MethodDefinitionInsertLocationFinder2 does not find implementation file when no definition already present. Patch by Marc-Andre Laperle. [*] BaseTestFramework.java 1.19 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.core.tests/regression/org/eclipse/cdt/core/tests/BaseTestFramework.java?root=Tools_Project&r1=1.18&r2=1.19 [*] MethodDefinitionInsertLocationFinder2.java 1.2 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt-core/org.eclipse.cdt.ui/src/org/eclipse/cdt/internal/ui/refactoring/implementmethod/MethodDefinitionInsertLocationFinder2.java?root=Tools_Project&r1=1.1&r2=1.2 [*] MethodDefinitionInsertLocationFinder.java 1.6 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt-core/org.eclipse.cdt.ui/src/org/eclipse/cdt/internal/ui/refactoring/implementmethod/MethodDefinitionInsertLocationFinder.java?root=Tools_Project&r1=1.5&r2=1.6 [*] GenerateGettersAndSettersRefactoring.java 1.20 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt-core/org.eclipse.cdt.ui/src/org/eclipse/cdt/internal/ui/refactoring/gettersandsetters/GenerateGettersAndSettersRefactoring.java?root=Tools_Project&r1=1.19&r2=1.20 [+] SourceHeaderPartnerFinder.java http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt-core/org.eclipse.cdt.ui/src/org/eclipse/cdt/internal/ui/editor/SourceHeaderPartnerFinder.java?root=Tools_Project&revision=1.1&view=markup [*] ToggleSourceAndHeaderAction.java 1.15 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt-core/org.eclipse.cdt.ui/src/org/eclipse/cdt/internal/ui/editor/ToggleSourceAndHeaderAction.java?root=Tools_Project&r1=1.14&r2=1.15 [*] ImplementMethod.rts 1.11 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.ui.tests/resources/refactoring/ImplementMethod.rts?root=Tools_Project&r1=1.10&r2=1.11 [*] GenerateGettersAndSetters.rts 1.11 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.ui.tests/resources/refactoring/GenerateGettersAndSetters.rts?root=Tools_Project&r1=1.10&r2=1.11 |
Created attachment 188837 [details] Improve search of implementation file Example: Test.h: class Test { int foo; }; Test.cpp: #include "Test.h" If you try to Generate and Setters, it won't find Test.cpp. The old MethodDefinitionInsertLocationFinder has a fallback that looks for a .cpp file. I think this could be improved by reusing code in the ToggleSourceAndHanderAction. I extracted some code from ToggleSourceAndHeaderAction and moved it to a new Util class, ToggleSourceAndHeaderUtil. Both 'Implement method' and 'Generate Getters and Setters' now use this code and I added some tests. The patch looks big but it's mostly moving stuff to a new Util class.