-
Notifications
You must be signed in to change notification settings - Fork 125
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
A flag to resolve populating index using ASTParser #2701
base: master
Are you sure you want to change the base?
Conversation
Introduce a SourceIndexer..DOM_BASED_INDEXER flag which instead of using the ECJ-derived parser uses ASTParser to create a DOM and then crawls this DOM to populate index. This has the benefit of: * Simplifying the code as an crawling DOM is usually simpler than dealing with lower-level parser * allowing other parsers as backend if ASTParser is configured for it
@@ -19,7 +19,7 @@ | |||
import org.eclipse.jdt.core.dom.CompilationUnit; | |||
|
|||
public class ASTHolderCUInfo extends CompilationUnitElementInfo { | |||
int astLevel; | |||
public int astLevel; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably better to introduce a public getter method.
@@ -88,6 +98,10 @@ public SourceIndexer(SearchDocument document) { | |||
} | |||
@Override | |||
public void indexDocument() { | |||
if (Boolean.getBoolean(getClass().getSimpleName() + ".DOM_BASED_INDEXER")) { //$NON-NLS-1$ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this flag per class vs a project or workspace preference? For a large project, this could result in a lot of system properties. As well, why are you are using the simple name as there could be collision with similarly named classes in different packages.
@@ -213,6 +227,15 @@ private void purgeMethodStatements(TypeDeclaration type) { | |||
|
|||
@Override | |||
public void indexResolvedDocument() { | |||
// TODO We need to rebuild the DOM with binding resolution enabled |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are the ramifications of having this code commented out? Can the PR be tested without it? You should have an issue cited here which is to fix the problem you are noting.
@@ -0,0 +1,371 @@ | |||
/******************************************************************************* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The title of this PR is a bit misleading. It implies you are just adding a new flag but you are adding this new code and this should be spelled out in the PR title and commit comment (e.g. Convert DOM to Index based on new flag setting)
What it does
Introduce a SourceIndexer.DOM_BASED_INDEXER flag which instead of using the ECJ-derived parser uses ASTParser to create a DOM and then crawls this DOM to populate index.
This has the benefit of:
How to test
Author checklist