Community
Participate
Working Groups
Currently, the CompletionProcessor2 does a full parse to get the ASTCompletionNode and we use resolvePrefix on the prefix IASTNames to find potention completion IBindings. This is still fairly slow, especially when referencing large headers. We should be able to get similar accuracy and a significant performance gain by using the SKIP_ALL_HEADERs mode of the parser and leveraging the Index (PDOM) to find all candidate IBindings. This bug will track that activity which will be done by my co-op student Bryan. Please post any comments, design ideas here.
Created attachment 57170 [details] proposed patch This patch modifies implementations of IASTName#resolvePrefix() so that they find bindings by not only navigating the AST but also by querying the index when necessary. This way, header files no longer need to be parsed for code completions. If, however, there is no indexer, the header files are still parsed to obtain the necessary information. To manage different content assist contexts, the task is actually delegated to the IASTName's closest ancestor that implements a new IASTCompletionContext interface. This patch also removes content assist's former case-sensitivity. Any feedback on the content assist behaviour and performance that this patch provides, or on my coding, would be much appreciated :)
(In reply to comment #1) > This patch also removes content assist's former case-sensitivity. > Any feedback on the content assist behaviour and performance that this patch > provides, or on my coding, would be much appreciated :) Just curious why the case sensitivity was removed? As far as C/C++ is concerned myFunction() != myfunction(). If I'm typing something and I want to complete it, I would expect that myF<completion> would not include myfunction(). If this is going to be the case, I'd rank the case sensitive matches higher than the case insensitive matches.
(In reply to comment #2) > Just curious why the case sensitivity was removed?... > > If this is going to be the case, I'd rank the case sensitive matches > higher than the case insensitive matches. A couple of reasons: 1) One of the main purposes for code completion is to speed up coding by saving keystrokes. Not needing to press the shift key to capitalize a letter may seem unimportant but it is a feature that I've definitely used while programming with the JDT. 2) Another of the main purposes would be to explore the tools (i.e. methods, fields, classes) at your disposal. It wouldn't be appropriate for one such tool to be hidden from the developer just because he/she chose the wrong case for a letter. I agree that the case-sensitive matches should be assigned a greater relevance, but currently all proposals are ranked identically (and sorted alphabetically), so there is a lot of work to be done in that respect.
I had a look at your patch. This looks like a very good solution to me! I have suggestions, some of them are strong (+), others are more or less just my opinion (-): + For searching the index you must use the BTree, otherwise content assist will not scale for larger projects. (IIndex.filterBindings(...)). + For searching namespace members you do use the BTree. The BTree is organized with a case-sensitive comparison, you cannot do a case-insensitive search. The mixure implemented in FindBindingsInBTree, will not work correctly. I do follow your arguments that it is best to use case-insensitive search. In order to achieve that we have to organize the BTrees differently, though. I suggest to do port the content assist in a case-sensitve manner first and raise another bug on this issue. + When proposals are obtained from a namespace scope in the AST, the index has to be searched in addition. Otherwise we will be missing proposals. + You should use IIndexBinding instead of PDOMBinding in CPPSemantics. + The PDOM version has to be increased, because you change the database layout. - I think IIndex.filterBindings(...) is not the best name. What about IIndex.findBindingsForPrefix(...) - I'd avoid introducing another filter object (BindingFilter). You can use IndexFilter and add a method acceptBinding(..). The BindingFilter contains functionality that does not make sense in the context of the IIndex API. - CharArrayUtil is for general purposes, it should not contain knowledge for content assist. CharArrayUtil.equals(..., prefixLookup) should be removed. - You have added a parameter to the method IScope.find(..). As this was public API in earlier versions you should leave the method and just add the second version. This way clients other than CDT don't have to change their code. - You have made a change to the LocationMap, which probably adresses a more general issue. Do we have a separate bugzilla for it?
Created attachment 57533 [details] revised patch Fixed up as per Markus's suggestions. Case-sensitivity is reimposed for the time being.
(In reply to comment #5) > Created an attachment (id=57533) [details] > revised patch > Fixed up as per Markus's suggestions. Case-sensitivity is reimposed for the > time being. The way you search the index looks fine. However with the updated patch, a lot of the automated tests are failing due to NullPointerExceptions. I think it's just minor stuff to clean up. The suites org.eclipse.cdt.core.suite.AutomatedIntegrationSuite and org.eclipse.cdt.ui.tests.AutomatedSuite should be passing or we should be able to explain, why we have a regression. Please also make sure to update the copyright notices. After that I'll be happy to apply the patch.
Created attachment 57624 [details] re-revised patch with fixes for J-Units Changes from the last patch include: - Checking that the index is non-null before using it :) - Updated a few tests: - BasicCompletionTest.testFunction() - CompletionTests.testGlobalFunctions_GlobalScope() - CompletionTests.testGlobalFunctions_MethodScope() - Converted failing failing tests into passing tests All remaining J-Unit errors/failures in the two suites are present with or without the patch for this bug.
Thanks for the great contribution, I have applied your patch.
Thanks, Markus. Great work Bryan. Keep 'em coming :)
