Bugzilla – Full Text Bug Listing
| Summary: | Implement method error | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Tools] CDT | Reporter: | Álvaro <alvarofleith> | ||||||||||||||||||
| Component: | cdt-refactoring | Assignee: | Marc-André Laperle <malaperle> | ||||||||||||||||||
| Status: | RESOLVED FIXED | QA Contact: | |||||||||||||||||||
| Severity: | normal | ||||||||||||||||||||
| Priority: | P3 | CC: | jamesblackburn+eclipse, malaperle, yevshif | ||||||||||||||||||
| Version: | 7.0 | ||||||||||||||||||||
| Target Milestone: | 8.0 | ||||||||||||||||||||
| Hardware: | All | ||||||||||||||||||||
| OS: | All | ||||||||||||||||||||
| Whiteboard: | |||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||
Hello Álvaro. I tried to reproduce the bug with the example you provided but it works for me. I've seen this bug many times (missing scope definition) but now that I want to fix it, I can't reproduce it. I will try to reproduce it in other scenarios but if you find a reliable way to reproduce it (reasonably sized), please let me know. Created attachment 173120 [details]
Workspace demonstrate bug
(In reply to comment #1) > Hello Álvaro. I tried to reproduce the bug with the example you provided but it > works for me. I've seen this bug many times (missing scope definition) but now > that I want to fix it, I can't reproduce it. I will try to reproduce it in > other scenarios but if you find a reliable way to reproduce it (reasonably > sized), please let me know. I add attachment for you reproduce the bug. The bug only occurs in linked folders containing source code. []', Álvaro. Created attachment 173163 [details]
Handle linked resources
Thank you for the attachment, I can now reproduce it. This patch fixes the problem.
Can you change the Component of the bug to cdt-refactoring? Thanks. (In reply to comment #5) > Can you change the Component of the bug to cdt-refactoring? Thanks. Yes. You are done! Created attachment 173336 [details]
Handle linked resources
Small tweak, findFilesForLocationURI handles both linked and non-linked resources.
Created attachment 186656 [details] Handle linked resources, updated Updated the patch after changes in bug 334051. Emanuel, does this look good to you? You should consider using CDT's ResourceLookup#selectFileForLocation or findFilesForLocation instead of calling straight through to the platform. Created attachment 186702 [details]
Handle linked resources, updated
Hi James, thanks for the suggestion. Is this patch better? I assume using ResourceLookup is better for performance?
I was thinking something more like:
IPath path = file.getLocation().removeFileExtension().addFileExtension("cpp"); //$NON-NLS-1$
IFile fileForLocation = ResourceLookup.selectFileForLocation(path, file.getProject());
ResourceLookup is designed for siutations such as these, it selects the most appropriate IFile based on CDT criteria (existing in the project, living under a source folder, etc.). It's nearly always wrong to do #findFilesForLocation()[0] as why should file 0 be preferred to file[1] or 2 or whatever...?
Apart from that you patch looks fine to me.
There are a couple things I don't understand:
- Why is ".cpp" a hard-coded extension? Surely any cpp content type should be accepted?
- Why does the code only handle files in the same directory?
I would have thought that any c++ file in the project (with the same name) should be considered?
Created attachment 188169 [details] Improve search of implementation file (In reply to comment #11) > There are a couple things I don't understand: > - Why is ".cpp" a hard-coded extension? Surely any cpp content type should > be accepted? > - Why does the code only handle files in the same directory? > > I would have thought that any c++ file in the project (with the same name) > should be considered? I have changed the patch to improve this. 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. In the case of 'Generate Getters and Setters', it was always failing to find an implementation file if there was no definition in it yet. This is due to the new MethodDefinitionInsertLocationFinder2 which doesn't use the .cpp 'fallback'. The patch looks big but it's mostly moving stuff to a new Util class. Created attachment 188836 [details]
Handle linked resources, updated
I'll create a new bug for the ToggleSourceAndHeaderUtil stuff, I think it's a bit out of the scope of this bug. This new patch uses selectFileForLocation as suggested. James, does this look good to you?
Created attachment 189387 [details] Handle linked resources, updated Updated the patch after bug 337040 was resolved. Now it's a one line change. Comment on attachment 189387 [details]
Handle linked resources, updated
Fixed in HEAD.
Fixed. (Didn't expect I'd have to comment) *** cdt cvs genie on behalf of mlaperle *** Bug 317686 - Implement method error. Fixed issue with linked resources. [*] NamespaceHelper.java 1.7 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.ui/src/org/eclipse/cdt/internal/ui/refactoring/utils/NamespaceHelper.java?root=Tools_Project&r1=1.6&r2=1.7 |
Forward error example using "Implement Method" in "Source menu": //Test.h file class Test { public: int implementTest(int i,int j) const; }; //Test.cpp file inline int implementTest(int i, int j) const { } Where is the scoped definition in implementation method ? inline definition is not necessary!! This correct is: int Test::implementTest(int i, int j) const { } -- Configuration Details -- Product: Eclipse 1.3.0.20100609-1425 (org.eclipse.epp.package.cpp.product) Installed Features: org.eclipse.platform 3.6.0.v20100602-9gF78GpqFt6trOGhL5t0nJy5fyGHKrwNY