Community
Participate
Working Groups
Build Identifier: So far, Context Assist only gave the results which were accessible in the given context at the place of invocation. If a given #include wasn't there, a symbol - usually - couldn't be accessed in CA. This behaviour is different from what we have in JDT. At any place, we have access to all classes which are accessible according to the given project's settings. When a type is selected, it automatically receives an `import` declaration to make it accessible. I thought that C/C++ coding would be faster if we could introduce similar behaviour: When a global symbol is selected from Context Assist, ensure that it is accessible (add include / forwarding declaration and handle the namespace properly). I've written a bit on this on my GSoC wiki page: http://wiki.eclipse.org/CDT/C_editor_enhancements/Include_management http://wiki.eclipse.org/CDT/C_editor_enhancements/Namespaces_and_%27using%27_management This feature needs to be discussed more IMHO, but I think that it would save a lot of time, especially when programming high-level OOP C++ with lots of namespaces and classes in separate files. There are also low-level projects in which manual handling of #includes is preferred (i.e. important order), so probably there could be a project-specific setting for this behaviour. Reproducible: Always
Created attachment 173695 [details] [WiP] initial prototype This is my first working prototype of this feature. Clearly work in progress, but works and gives an idea what could be done here. Changes: + Context Assist proposal extended with an index query for globally accessible bindings (i.e. in global or namespace scope) + When choosing an option from Context Assist, `add include` action is triggered - it also adds `using namespace::symbol` when needed. Known issues: - Cursor is placed in wrong position - Add Include should be faster; this shouldn't hurt CA performance - Local proposals should be prioritized over global proposals (?) Things to think about: * Is the index query for global symbols correct and optimal? Should they be filtered less or more? * What are the known issues with `Add include` action? * Alternative ways of handling symbols in namespaces (see wiki on namespace handling) - depending on .cpp/.h context, or maybe project specific? * Any clever way of determining when to insert a forwarding declaration instead of an #include?
(In reply to comment #0) > Build Identifier: > > So far, Context Assist only gave the results which were accessible in the given > context at the place of invocation. If a given #include wasn't there, a symbol > - usually - couldn't be accessed in CA. This is not true, content assist proposes bindings even if the header is not included. What makes you think this is different?
(In reply to comment #2) Yes, you are right. But the context assist doesn't suggest bindings in namespace scope, without specifying that namespace explicitly. This would mean that the only extension needed for CA would be "globally accessible symbols in namespace scope", as global scope is already handled.
(In reply to comment #3) > (In reply to comment #2) > > Yes, you are right. > But the context assist doesn't suggest bindings in namespace scope, without > specifying that namespace explicitly. Proposing members of a namespace would be quite surprising, we'll get bug reports on that pretty quick. > This would mean that the only extension needed for CA would be "globally > accessible symbols in namespace scope", as global scope is already handled. What do you mean with 'globally accessible"? The concept of accessibility only applies to class members. If you meant to consider the visibility (different concept) then clearly none of the namespace members are visible unless you put a using declaration into your code. In this case content assist will pick up the members: E.g.: #using namespace std; // content assist will propose string, vector, ...
(In reply to comment #4) Yes, that's how namespace visibility is handled now. I suggested another approach on the wiki, treating namespaces in C++ as packages in Java and having `using namespace::symbolname` added automatically by Context Assist as JDT does with `import package.typename`.
(In reply to comment #5) > (In reply to comment #4) > Yes, that's how namespace visibility is handled now. I suggested another > approach on the wiki, treating namespaces in C++ as packages in Java and having > `using namespace::symbolname` added automatically by Context Assist as JDT does > with `import package.typename`. I am sure that for many users, the current model is the correct one. Adding using directives to a file can change a lot and may be undesired. So I think the suggested feature should be optional. This could be similar to proposals that require a static import in JDT, see preference page Java - Editor - Content Assist - Favorites. With that we'd basically have a list of additional 'using namespace' directives to be considered.
(In reply to comment #6) Yes, I've mentioned in the description that this needs some settings, optimally global + project-specific. Probably several options, my idea is something like: [x] Add include automatically upon DOM proposal selection (checkbox) [x] Propose symbols from all namespaces (checkbox) Behaviour upon selection: (list) --- `using namespace::symbolname` (verbose) --- Append `namespace::` before the inserted symbol (safe) --- Verbose style in module files, safe style in headers (default?) This would make it possible to work either with desired level of support for includes/namespaces from IDE, or manually, as it is now. I'd like a bigger feasibility study for this.
(In reply to comment #7) > ... > [x] Propose symbols from all namespaces (checkbox) > Behaviour upon selection: (list) > --- `using namespace::symbolname` (verbose) > --- Append `namespace::` before the inserted symbol (safe) > --- Verbose style in module files, safe style in headers (default?) The options look good, however I am not sure how useful it is to choose between all or no namespaces. Headers for libraries typically contain namespaces that are not intended to be used by clients. For example in the standard headers of gcc you find the namespaces '__debug', '__gnu_cxx', '__gnu_debug', '__cxx_abiv1' and probably many more. I am pretty sure you do not want to have members of these namespaces proposed in the same way as other global entities. Therefore I think it may be better to have the user explicitly list the namespaces that should be searched. In a typical use case a user would list namespaces like 'std' or 'boost'. This compares nicely with the JDT feature for static imports (Preference Page Java - Editor - Content Assist - Favorites).
(In reply to comment #8) > Therefore I think it may be better to have the user explicitly list the > namespaces that should be searched. In a typical use case a user would list > namespaces like 'std' or 'boost'. Good point, I agree. Having to set this one time in the IDE is not a big burden for the user. I'll try this out this week. ~~~~ Besides, about automatic triggering of `add include` upon Context Assist completion, I've found something like this: /org.eclipse.cdt.ui/src/org/eclipse/cdt/internal/ui/text/contentassist/ContentAssistPreference.java:65: // /** Preference key for adding includes on code assist (unused) */ // public final static String ADD_INCLUDE= "content_assist_add_import"; //$NON-NLS-1$ It's commented out and also referenced in commented-out code sometimes; it seems like someone had been working on that before. A glance at the history reveals that it was removed from preference page in 24-01-2007, so that's a bit old. However, can we somehow verify if that feature existed sometime and why it was removed?
Created attachment 174578 [details] [WiP] prototype with favorite namesapces After creating the menu pane in bug 320159, I've made the CA look for proposals in those namespaces. Lookup works fine, but practice reveals problems with Add Include which would make this barely usable. That park certainly needs more work.
(In reply to comment #9) > .... > Besides, about automatic triggering of `add include` upon Context Assist > completion, I've found something like this: > /org.eclipse.cdt.ui/src/org/eclipse/cdt/internal/ui/text/contentassist/ContentAssistPreference.java:65: > // /** Preference key for adding includes on code assist (unused) */ > // public final static String ADD_INCLUDE= "content_assist_add_import"; > //$NON-NLS-1$ > It's commented out and also referenced in commented-out code sometimes; it > seems like someone had been working on that before. > .... Many parts of CDT started as a clone of the counterpart in JDT. The preference was simply copied and commented away later on.
(In reply to comment #10) > Created an attachment (id=174578) [details] > [WiP] prototype with favorite namesapces > After creating the menu pane in bug 320159, I've made the CA look for proposals > in those namespaces. > Lookup works fine, but practice reveals problems with Add Include which would > make this barely usable. That park certainly needs more work. As it is done in the patch, the additional lookup in the namespaces bypasses some logics that is implemented in the various nodes behind the interface IASTCompletionContext. For example, content assist will not propose variables in places where you cannot use them. Therefore I think the additional namespaces need to be passed all the way to CPPSemantics.contentAssistLookup(LookupData data, IScope start) and need to be handled there.
That's right, this needs to be changed. However, the preference belongs to the UI and the lookup is done by the core. The request for bindings is passed from UI to core via IASTCompletionContext, which is API, so I cannot extend it - shall I create a second interface on top of it (and have it implemented in current IASTCompletionContext implementations) for the purpose of passing the namespace list to core? I can't think of another solution.
Just noticed that IASTCompletionContext has @noextend and @noimplement, so maybe I could just add another method there or possibly add the parameter to the existing method (and update all the implementations accordingly)?
(In reply to comment #14) > Just noticed that IASTCompletionContext has @noextend and @noimplement, so > maybe I could just add another method there or possibly add the parameter to > the existing method (and update all the implementations accordingly)? You cannot change the existing method (that's an API breakage because there may be clients using the old method), however you can add a new method. The new functionality is specific to c++, therefore we can also add a new interface, ICPPASTCompletionContext, that derives from IASTCompletionContext and adds the new method. With that you don't have to change the c-specific nodes that implement IASTCompletionContext.
Created attachment 174922 [details] ICPPASTCompletionContext introduced and implemented the new interface for C++ fav namespace lookup
Created attachment 174923 [details] improved lookup on favorite namespaces The lookup seems to be correct now. Also, introduced correct handling of nested namespaces.
Created attachment 175766 [details] ICPPASTCompletionContext + changes to CPPSemantics * The methods CPPSemantics.findBindings(..) perform a standard lookup, which is not limited to the provided scope. I have changed the lookup, it now also avoids multiple lookups in the same namespace. You may want to review my changes to CPPSemantics. * I have added the testcase (AST2CPPTests.testAdditionalNamespaceLookup()). It'd make sense to add some more testcases to check whether the patch works for nested namespaces. (e.g.: With 'ns::nested' we should propose stuff from 'nested' but not from 'ns').
(In reply to comment #17) > Created an attachment (id=174923) [details] > improved lookup on favorite namespaces > > The lookup seems to be correct now. > Also, introduced correct handling of nested namespaces. Note that IBindings may not leave the scope of the index lock, because they become invalid when the lock is released. CDOMCompletionProposal currently stores an IBinding as a member. This member is not used anywhere atm, but leaking an IBinding like this is a no-go. I assume the proposal implementation is work in progress (considering the discussion with Sergey on cdt-dev), but I thought an early heads-up would not hurt.
Yes, Anton, that part remained from my sandboxing. Thanks for pointing that out :) --- So let's clarify what we have here at the moment. "Add Include" was discussed on the list by Sergey and me and probably would need some refactoring and reimplementing, after which it would finally be reliable enough to have it bound on Context Assist. About this part- after we have some cleanup here, do you find it suitable to commit it? I'd +1 here, because favourite namespace lookup: a) is optional, b) seems to run reliably, c) is actually useful if we accept the need for the user to add `using`s either manually, or by hitting Add Include after selecting nominated namespace proposal (which is kind-of-reliable now for most cases). So shall we clean this up and have it committed alongside with bug 320159? Reimplementing the Add Include feature and having it triggered after Context Assist looks like another bug.
To make some progress, I have committed the changes in the parser. The actual content assist still needs to be hooked up.
*** cdt cvs genie on behalf of mschorn *** Bug 319175: Parser adds proposals for preferred namespaces. [*] CPPSemantics.java 1.172 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt-core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/dom/parser/cpp/semantics/CPPSemantics.java?root=Tools_Project&r1=1.171&r2=1.172 [*] CPPASTFieldReference.java 1.32 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt-core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/dom/parser/cpp/CPPASTFieldReference.java?root=Tools_Project&r1=1.31&r2=1.32 [*] CPPASTNamedTypeSpecifier.java 1.19 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt-core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/dom/parser/cpp/CPPASTNamedTypeSpecifier.java?root=Tools_Project&r1=1.18&r2=1.19 [*] CPPASTQualifiedName.java 1.49 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt-core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/dom/parser/cpp/CPPASTQualifiedName.java?root=Tools_Project&r1=1.48&r2=1.49 [*] CPPASTConstructorChainInitializer.java 1.19 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt-core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/dom/parser/cpp/CPPASTConstructorChainInitializer.java?root=Tools_Project&r1=1.18&r2=1.19 [*] CPPASTBaseSpecifier.java 1.20 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt-core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/dom/parser/cpp/CPPASTBaseSpecifier.java?root=Tools_Project&r1=1.19&r2=1.20 [*] CPPASTIdExpression.java 1.25 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt-core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/dom/parser/cpp/CPPASTIdExpression.java?root=Tools_Project&r1=1.24&r2=1.25 [*] CPPASTUsingDeclaration.java 1.19 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt-core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/dom/parser/cpp/CPPASTUsingDeclaration.java?root=Tools_Project&r1=1.18&r2=1.19 [*] CPPASTUsingDirective.java 1.18 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt-core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/dom/parser/cpp/CPPASTUsingDirective.java?root=Tools_Project&r1=1.17&r2=1.18 [*] CPPASTName.java 1.38 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt-core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/dom/parser/cpp/CPPASTName.java?root=Tools_Project&r1=1.37&r2=1.38 [+] ICPPASTCompletionContext.java http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt-core/org.eclipse.cdt.core/parser/org/eclipse/cdt/core/dom/ast/ICPPASTCompletionContext.java?root=Tools_Project&revision=1.1&view=markup [*] AST2CPPTests.java 1.351 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt-core/org.eclipse.cdt.core.tests/parser/org/eclipse/cdt/core/parser/tests/ast2/AST2CPPTests.java?root=Tools_Project&r1=1.350&r2=1.351
(In reply to comment #20) > So shall we clean this up and have it committed alongside with bug 320159? > Reimplementing the Add Include feature and having it triggered after Context > Assist looks like another bug. Yes, I think this should be good to go after the cleanup (the patch for bug 320159 also needs a few changes). It would be good to have a few JUnit tests for that feature. See CompletionTests in org.eclipse.cdt.ui.tests.
It looks like the patch here was committed. Any reason this bug is still open?
Closing per comment 24.
