Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.
Bug 337040 - MethodDefinitionInsertLocationFinder2 does not find implementation file when no definition already present
Summary: MethodDefinitionInsertLocationFinder2 does not find implementation file when ...
Status: RESOLVED FIXED
Alias: None
Product: CDT
Classification: Tools
Component: cdt-refactoring (show other bugs)
Version: 8.0   Edit
Hardware: All All
: P3 normal (vote)
Target Milestone: 8.0   Edit
Assignee: Sergey Prigogin CLA
QA Contact: Emanuel Graf CLA
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 292851
  Show dependency tree
 
Reported: 2011-02-12 13:30 EST by Marc-André Laperle CLA
Modified: 2012-05-22 14:53 EDT (History)
2 users (show)

See Also:


Attachments
Improve search of implementation file (29.70 KB, patch)
2011-02-12 13:30 EST, Marc-André Laperle CLA
no flags Details | Diff
Improve search of implementation file (34.12 KB, patch)
2011-02-18 02:15 EST, Marc-André Laperle CLA
eclipse.sprigogin: iplog+
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Marc-André Laperle CLA 2011-02-12 13:30:41 EST
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.
Comment 1 Marc-André Laperle CLA 2011-02-12 13:38:06 EST
Hi Sergey, this is the last (major?) issue that I'm aware of with the new Generate Getters and Setters.
Comment 2 Sergey Prigogin CLA 2011-02-13 00:11:42 EST
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
Comment 3 Marc-André Laperle CLA 2011-02-18 02:15:47 EST
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!
Comment 4 Sergey Prigogin CLA 2011-02-19 16:50:19 EST
Fixed in HEAD > 20110219.
Comment 5 CDT Genie CLA 2011-02-19 17:23:08 EST
*** 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