Community
Participate
Working Groups
Build Identifier: 201005061048 I had this code: apply_any_parameter_constraints(expectation, (const char*)cgreen_vector_get(names, i), va_arg(actual, intptr_t)); I highlighted this portion: (const char*)cgreen_vector_get(names, i) and hit Alt+Shift+L to extract a local variable. I entered 'parameter_name' for the variable name and hit enter. Eclipse produced this code: void parameter_name = (const char*)cgreen_vector_get(names, i); apply_any_parameter_constraints(expectation, (const char*)cgreen_vector_get(names, i), va_arg(actual, intptr_t)); I was expecting it to pick up on the explicit cast for the new variable declaration's type, and to replace the originally highlighted expression and produce this: const char* parameter_name = (const char*)cgreen_vector_get(names, i); apply_any_parameter_constraints(expectation, parameter_name, va_arg(actual, intptr_t)); Note: this is from the open source unit testing framework called cgreen. Reproducible: Always Steps to Reproduce: 1. type in this program #include <stdio.h> int main(char** argv, int argc) { printf("%s\n", (char*)argv[0]); } 2. highlight (char*)argv[0] 3. hit Alt+Shift+L 4. type in a name, hit enter Note: this reproduces the lack of typing of the new variable only. I could reproduce the problem of the extracted expression not being replaced correctly within the context of the cgreen code.
Additional note: This fails in the same way even when I change the (char*)argv[0] to argv[0]. Eclipse still knows the underlying type, either way.
I've gotten several complaints from local developers here about this issue, as it produces code that doesn't parse/compile correctly. I'm upgrading it to Major as a result.
I believe the problem lies there: org.eclipse.cdt.internal.ui.refactoring.extractfunction.ExtractExpression.handleLiteralExpression(IASTLiteralExpression) This class has several problems besides the one you mentioned: - (major): It returns a IASTDeclSpecificier, which contains only the part of type, which makes it impossible to infer more complex types like pointers or arrays - (minor): treats string literals as consisting of wchar_t instead of char. The latter is a trivial fix (done), I'm working at the former. Both of those can be illustrated by an example: int main(int argc, char* argv[]) { const char *c = "something"; } Mark 'something' and run "Extract local variable" refactoring. You'll get: wchar_t asd = "something"; This problem affects `Extract local variable`, `Extract function` and possibly `Extract constant` - not sure on the last. The situation you mentioned seems to be yet another issue in the same class, I'll have a look.
Success! I've made a new class for inferring types. Give me some time to put it in the right place and bind it to the refactorings and we're done here. I'll drop a patch soon.
Created attachment 172652 [details] DeclarationGenerator module created and applied in ExtractLocalVariableRefactoring Here's the patch. It introduces a new class for generating declarations using an expression type. I believe that this class may also come in handy in Extract Constant and Extract Function refactorings, which suffer from similar problems at the moment. Is it the same bug or should a new one be opened for that?
(In reply to comment #5) > Here's the patch. It introduces a new class for generating declarations using > an expression type. Can I download this in a 7.0 nightly build? I don't have the environment set up to build CDT itself. > I believe that this class may also come in handy in Extract Constant and > Extract Function refactorings, which suffer from similar problems at the > moment. Is it the same bug or should a new one be opened for that? I'm not sure. I'll test a bunch of cases once I get and install the archive, then open new bugs for anything I notice. Thanks!
(In reply to comment #5) > Created an attachment (id=172652) [details] > DeclarationGenerator module created and applied in > ExtractLocalVariableRefactoring > > Here's the patch. It introduces a new class for generating declarations using > an expression type. Hi Tomasz, thank you for working on this :). I glanced quickly over the patch, I just have a couple of suggestions: 1. I don't see any fields in DeclarationGenerator. Maybe the methods could be made static? That would remove the need to instantiate a DeclarationGenerator. 2. I see many org.eclipse.cdt.internal stuff, I believe you can now use a factory to be API clean, at least that's what I use now. Look at org.eclipse.cdt.core.dom.ast.ASTNodeFactoryFactory. (In reply to comment #6) > Can I download this in a 7.0 nightly build? I don't have the environment set up > to build CDT itself. Not yet, this has to be committed first. Once it's in, a committer will add a comment here. It'll probably go in HEAD (8.0) since there is a new public class. I don't think 8.0 is being built yet so you'd have to check out from CVS to try it once it's committed. http://wiki.eclipse.org/Getting_started_with_CDT_development
(In reply to comment #5) > Created an attachment (id=172652) [details] > DeclarationGenerator module created and applied in > ExtractLocalVariableRefactoring > > Here's the patch. It introduces a new class for generating declarations using > an expression type. Thanks for the patch. I'll have a look at your patch next week.
(In reply to comment #7) > 1. I don't see any fields in DeclarationGenerator. Maybe the methods could be > made static? That would remove the need to instantiate a DeclarationGenerator. Definitely. I just took the suggestion from ExtractExpression class, but I agree with you. > 2. I see many org.eclipse.cdt.internal stuff, I believe you can now use a > factory to be API clean, at least that's what I use now. Look at > org.eclipse.cdt.core.dom.ast.ASTNodeFactoryFactory. Oh, so THAT's the `standard` non-internal way of creating AST nodes. Thanks! I'll change the class and submit a new patch today . > Not yet, this has to be committed first. Once it's in, a committer will add a > comment here. It'll probably go in HEAD (8.0) since there is a new public > class. Probably you're referring to CDT's policy towards `API changes`? I'm new to CDT development, but I believe that this should be made available for Helios users sometime soon. Note that: - Even though it's a new class, it's introduced for the purpose of a bugfix (the new refactorings in CDT are basically broken at the moment) - It's made public mainly because other components besides refactoring will need it soon (well, my w.i.p. Codan fixes do already) - It's only a new class, so clearly a backwards-compatible change (won't break client code)
I recommend adding the class as an internal class (on the 7.0.x branch this is the only way to add it). If necessary the class can be made API later on at any time, however once it is API you have a hard time making changes to it. Note that if you make it API you need to tag the class as @noextend. Also, for API I recommend to separate the implementation from the API. It is much cleaner if you present just the public methods in the API. (For example: IASTDeclSpecifier createDeclSpecFromType(IType type, IASTNodeFactory factory) { return DeclarationGeneratorImpl.createDeclSpec(type, factory); }
(In reply to comment #9) > (In reply to comment #7) > > 2. I see many org.eclipse.cdt.internal stuff, I believe you can now use a > > factory to be API clean, at least that's what I use now. Look at > > org.eclipse.cdt.core.dom.ast.ASTNodeFactoryFactory. > > Oh, so THAT's the `standard` non-internal way of creating AST nodes. Thanks! > I'll change the class and submit a new patch today . Most of the time you can avoid using the ASTNodeFactoryFactory because you can obtain the factory from the AST via IASTTranslationUnit.getNodeFactory().
Created attachment 172810 [details] DeclarationGenerator module and IDeclaratorGenerator interface created and applied in ExtractLocalVariableRefactoring I'm submitting an updated patch. I followed your advice and extracted the interface, then moved the implementation to internal package. I also changed the code to make use of factories. One more thing - since DeclarationGenerator concrete class is now internal, some API should probably be responsible for instantiation of this class (given a INodeFactory). Any ideas here?
(In reply to comment #12) > One more thing - since DeclarationGenerator concrete class is now internal, > some API should probably be responsible for instantiation of this class (given > a INodeFactory). Any ideas here? The API itself can also be a class (rather than an interface), then you can use static methods without the need for a factory.
Created attachment 172942 [details] DeclarationGenerator and DeclaratorGeneratorImpl created and applied in ExtractLocalVariableRefactoring I understand this idea. I did as you suggested, Markus. I hope it's OK now.
(In reply to comment #14) > Created an attachment (id=172942) [details] > DeclarationGenerator and DeclaratorGeneratorImpl created and applied in > ExtractLocalVariableRefactoring > > I understand this idea. I did as you suggested, Markus. I hope it's OK now. Thanks, the API itself is cleaned up. I have not looked at the actual implementation, Emanuel is going to do that.
Extract local variable refactoring has bunch of tests. If you can update them with new one that are supported now and they all pass, I will commit it.
Created attachment 173123 [details] test case update Done. Also see bug 318493 for the currently failing case
fixed in 8.0 > 20100701
*** cdt cvs genie on behalf of egraf *** Bug 312736: extract variable refactoring does not infer type or replace expression correctly <a href=https://bugs.eclipse.org/bugs/show_bug.cgi?id=312736>https://bugs.eclipse.org/bugs/show_bug.cgi?id=312736</a> [+] DeclarationGeneratorImpl.java http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/dom/rewrite/DeclarationGeneratorImpl.java?root=Tools_Project&revision=1.1&view=markup [+] DeclarationGenerator.java http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.core/parser/org/eclipse/cdt/core/dom/rewrite/DeclarationGenerator.java?root=Tools_Project&revision=1.1&view=markup [*] ExtractLocalVariable.rts 1.4 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.ui.tests/resources/refactoring/ExtractLocalVariable.rts?root=Tools_Project&r1=1.3&r2=1.4 [*] ExtractLocalVariableRefactoring.java 1.8 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.ui/src/org/eclipse/cdt/internal/ui/refactoring/extractlocalvariable/ExtractLocalVariableRefactoring.java?root=Tools_Project&r1=1.7&r2=1.8
Can this be backported to 7.0.x? Most of the new refactorings in 7.0 are pretty much broken without this fix. Thanks!
Patch as it is no - because it introduces new API, if it is redone to fill of it in ExtractLocalVariableRefactoring - maybe but it would a hack
(In reply to comment #21) > Patch as it is no - because it introduces new API The fact that the new functionality from this bugfix was exposed via API is that another component (Codan) is going to use it. It could be done as well as internal to refactoring, but then it'd have to be either duplicated in Codan or exposed anyway. What's the point? Introduction of a new API is a backward-compatible change, so it won't break anything. It seems to me like the formal CDT process actually prohibits us to deliver a demanded bugfix to our users. Is something wrong here?
It is not CDT policy it is eclipse. It is versions. If only minor version changed no new APIs are allowed even they are extensions not api breakages. I don't think I agree with this either but it is a policy. If you want to fix in in 7.0.1 for refactoring - it is ok. Changes in Codan will go to 8.0 only anyway.
So let's clarify - if we want this bugfix (and other refactoring bugfixes dependent on DeclarationGenerator) to be applied for CDT 7 users, how should it be done? I'm not sure how the process handles performing updates to 7.0 but my idea is to: * Create a patch ONLY for 7.0 branch with: - DeclarationGeneratorImpl copied into org.eclipse.cdt.refactoring, marked as @Deprecated and commented that in 8.0 it shall be accessed via API in org.eclipse.cdt.core.dom.rewrite * Leave the HEAD branch as it is now, as it corresponds to CDT 8.0 ^ Something like this? Do I understand correctly that if we apply a patch to 7.0 branch, it will be delivered to 7.0 users but will never be merged into head branch? Head already has the `correct` approach with updated API so we don't want it to be affected.
(In reply to comment #24) > > * Create a patch ONLY for 7.0 branch with: > - DeclarationGeneratorImpl copied into org.eclipse.cdt.refactoring, marked as > @Deprecated and commented that in 8.0 it shall be accessed via API in > org.eclipse.cdt.core.dom.rewrite DeclarationGeneratorImpl is in a internal package so it can stay there. > > * Leave the HEAD branch as it is now, as it corresponds to CDT 8.0 > > > ^ Something like this? Do I understand correctly that if we apply a patch to > 7.0 branch, it will be delivered to 7.0 users but will never be merged into > head branch? Head already has the `correct` approach with updated API so we > don't want it to be affected. Yes changes to the 7.0 branch aren't merged automatically. If you want to fix a bug in both branches the patch needs to be applied to both branches manually.
(In reply to comment #25) > DeclarationGeneratorImpl is in a internal package so it can stay there. Okay, but DeclarationGeneratorImpl is internal for cdt.core, while refactorings reside in cdt.ui - we'd get some warnings about discouraged access, I guess. Is this OK?
(In reply to comment #26) > (In reply to comment #25) > > DeclarationGeneratorImpl is in a internal package so it can stay there. > > Okay, but DeclarationGeneratorImpl is internal for cdt.core, while refactorings > reside in cdt.ui - we'd get some warnings about discouraged access, I guess. Is > this OK? The plug-in org.eclipse.cdt.ui is a friend of org.eclipse.cdt.internal.core.dom.rewrite so we get no warning. I'm backporting your patch at the moment. I'll mark the class as EXPERIMENTAL.
Please also use the update from bug 318784.
I committed the patch without the API change to 7.0. I'll have a look at Bug 318784 and Bug 318785 tomorrow. Changing target Milestone to 7.0.1.
*** cdt cvs genie on behalf of egraf *** Bug 312736 patch without public API change. [*] ExtractLocalVariable.rts 1.3.6.1 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.ui.tests/resources/refactoring/ExtractLocalVariable.rts?root=Tools_Project&r1=1.3&r2=1.3.6.1 [+] DeclarationGeneratorImpl.java http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/dom/rewrite/DeclarationGeneratorImpl.java?root=Tools_Project&revision=1.1&view=markup [*] ExtractLocalVariableRefactoring.java 1.7.2.1 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.ui/src/org/eclipse/cdt/internal/ui/refactoring/extractlocalvariable/ExtractLocalVariableRefactoring.java?root=Tools_Project&r1=1.7&r2=1.7.2.1
