Community
Participate
Working Groups
This is "short" explanation of our enhancement request. A full length explanation follows after the Easter weekend. Changes in the core: To provide our refactorings we need some more information than you provide with your scanner and parser at the moment. That's the reason why we have to add a few things in the core. We tried to keep the chances in the core as small as possible. Description of the changes: Scanner: To generate the source from the AST we added the missing information to several preprocessor nodes. The Scannerconfiguration contains a flag if the comments should be skipt or returned as a token. Parser: We created a new parser-subclass. This new parser adds comments to existing Nodes. Additionaly we created a new object for all nodelocation relateted numbers. To generate the the change objects in the refactorings we often need the fileoffset instead of the translationunit offset so we added this offset to the new location object. Additional refactorings: This patch adds the following refactorings to the CDT: - ExtractMethod - Extract Constant - Extract Subclass - Move Field - Move Method - Hide Method But for the enhancements we need the changes from our core patch. Because we use our parser to get the needed information. To actually support changes in the source code we first made the AST writable. The structure of the ASTWriter is inspired by the JDT project. So with this ASTWriter we provide a code generator which is also useful for many other things. We also implemented a pseudo-preprocessor which should solve the problem with the preprocessor statements as described in the paper from Alejandra Garrido. The pseudo-processor is not finished and is not used by the refactorings. It was just a spike to find out what's possible and where the problems are. A small backdraw: This patch contains Java 1.5 code and we haven't changed it. With regard to the next release (Ganymed) which will hopefully be Java 1.5. Unless you think our contributions should go in to the europa release we would be glad to change it to java 1.4 code. Otherwise we will provide a patch for the next release which can be included in the CDT without such big timeconstraints. Of course we will add some more refactorings till then. Generally: It may sound strange that you should use a patch with no direct value at the moment. But for us it's the only way to provide a Plug-in with our refactoring support. Otherwise we have to spread our own CDT version with the included refactorings. Which would be really awful. And besides you maybe will be also happy to have this information in the parser. Especially for the formatter and other parts of the CDT which make modification to the source code. For us it's a fundamental need, to getting further with our development. So we would truly appreciate if you take the core patch into the europa release.
Created attachment 63210 [details] Changes in org.eclipse.cdt.core
Created attachment 63211 [details] Changes in org.eclipse.cdt.core.tests
Created attachment 63212 [details] Our refactoring Plug-in including the test Plug-in We created our own refactoring Plug-in because of a dependency problem. See http://dev.eclipse.org/mhonarc/lists/cdt-dev/msg08810.html
I have had a look at the core-patch, here are my suggestions: The AST holds on to character arrays rather than String objects and to arrays in general rather than Collections. As far as I know this has been done in order to keep the memory footprint as low as possible. Anyways, I think this pattern should be followed, just to have it 'all the same'. Certainly using objects of type 'Vector' is not a good idea, as those are synchronized. The AST is not thread-safe, so this adds just a performance-overhead with no gain. You followed a bad CDT-habit of not documenting public API. I personally will not commit code with new undocumented public API. I suggest to break up the requested changes into several bugs: (1) Adding more information to the preprocessor nodes. (2) Adding comments to the AST. (3) Fixing up the location information in the AST. From my point of view (1) should be no problem. Ad (2): Adding comments to the AST should not be done by a different parser. A different parser should only be used for a differnet language (variant). I suggest to integrate the work in the regular CDT-parsers in a way that you can turn this on or off. Note that you can already pass options to AbstractLanguage.getASTTranslationUnit(...). This addition to the parser would be very useful for CDT, as we could base the task-list population on this. Ad (3): The implementation of the location information is certainly not working for many cases but it can provide you the fileoffset if there is one. I don't yet understand why you want to change the API here. Furthermore your API does not support the case where there is no file-offset. Please describe the kind of problem the current solution is causing you. I don't support the idea of putting any of this into the 4.0 release, it's just too late in the game. I will also look into the refactoring plugin you provided at a later point. My hope is that you can break out stuff from there into separate bugs as well (e.g. the AST-rewriter). Smaller pieces can be integrated into CDT much faster and it also gives you a chance to build up a record of contributions. I expect the next CDT version to require java 1.5, so there should not be a need to port your work to 1.4. It should go directly into the core- and ui- plugins, with that there is no dependency problem. I think we'll remove the separate refactoring-plugin in the next CDT-release, anyhow.
Thanks for your feedback Markus. I created a new report for adding additional information to the preprocessor nodes. https://bugs.eclipse.org/bugs/show_bug.cgi?id=182889
Adding a new dependency did't work as expected. Next try.
We'll adjust our refactoring work to HEAD and split it into smaller pieces. Let me now if I can be of any help with the redesign of the existing refactoring plugin.
(In reply to comment #7) > We'll adjust our refactoring work to HEAD and split it into smaller pieces. Let > me now if I can be of any help with the redesign of the existing refactoring > plugin. So far I don't have plans for to rework the rename-refactoring other than moving it from the separate plugin into the cdt.core and cdt.ui plugins (after 4.0). When you add more refactorings there probably need to be changes in order to have a good overall-architecture. Any proposals on how to move this on are welcome.
*** Bug 75364 has been marked as a duplicate of this bug. ***
Comment on attachment 63212 [details] Our refactoring Plug-in including the test Plug-in We'll adapt the refactorings to the new AST modifaction api.
Created attachment 90857 [details] Extract constant refactoring This is the first refactoring we have adapted to the new ASTModification API. We'll adapt the rest of our refactorings to the new API and contribute them later. We used this refactoring to test the new API and the implementation of the change generator.
(In reply to comment #11) > Extract constant refactoring Thanks I'll take care of getting permission for checkin, a few things to do (I can handle that before applying the patch): * copy right notice should show year 2007 * rename package org.eclipse.cdt.internal.ui.refactoring.ui to org.eclipse.cdt.internal.ui.refactoring.dialogs * comment in German * spelling errors in message.properties
Created attachment 92278 [details] documentation patch for the previous patch This patch adds some documentation to the previous patch. Needs to be applied after the first one.
(In reply to comment #11) > Created an attachment (id=90857) [details] I have applied the patch with a couple of changes: * There was an existing CTextFileChange that allows the CModel to participate in the changes, I made sure this one gets used and removed yours. * I removed the action implementations in favour of a simpler one that is added to the CRefactoringActionGroup programatically. Also I defined and assigned a command-id and provided a kbd-shortcut (ALT-C, as in JDT). * I have changed the AST generation to create an index based AST. This way the header files do not have to be parsed. Please double-check this change. I think we should close this bug and deal with further refactorings in a bug per refactoring.
(In reply to comment #13) > Created an attachment (id=92278) [details] > documentation patch for the previous patch > This patch adds some documentation to the previous patch. Needs to be applied > after the first one. Thanks, I have applied this patch. However, due to my changes not all of the changes matched and are left unchanged: * FileHelper, I don't know why it did not match * Messages, I guess because I renamed one of the packages.
(In reply to comment #14) > * I have changed the AST generation to create an index based AST. This way the > header files do not have to be parsed. Please double-check this change. thanks Markus I guess most of the refactorings will need comments in the translation unit. So maybe we should add the AST_CREATE_COMMENT_NODES flag to the AST_STYLE constant.
We are now handling each refactoring in a separate bug: e.g.: bug 226658, bug 226646, bug 226490, bug 226484. Marking this bug as fixed.
assigning
done
