Skip to content

Conversation

@robstryker
Copy link
Contributor

ICompilationUnit.getCustomOptions() has the following javadoc:

/**

  • Returns the table of the current custom options for this ICompilationUnit. If there is no setOptions called
  • for the ICompliationUnit, then return an empty table.
  • @return the table of the current custom options for this ICompilationUnit
  • @SInCE 3.25
    */

If the IElementInfo has not yet been loaded or does not yet exist, then nobody could have possibly called setOptions on it. Options on a per-cu basis are not persisted in any way, so the only way to set options on a unit is if it's element info has been loaded.

One of the side effects of loading the element info is that source indexer gets called or an attempt to build the structure of the file gets initiated, which may parse the file in order to do that. In many cases, the entire purpose of calling getCustomOptions is to discover what options should be used to parse the file. If the act of calling getCustomOptions to use when parsing the file has the side effect of parsing the file beforehand, then the work of parsing is done twice to no benefit.

For example:

		ASTParser astParser = etc;
		astParser.setWorkingCopyOwner(cu.getOwner());
		astParser.setSource(this instanceof ClassFileWorkingCopy ? source : cu);
		astParser.setProject(cu.getJavaProject());
		astParser.setCompilerOptions(cu.getOptions(true));
		ASTNode dom = null;
		try {
			dom = astParser.createAST(pm);

In this example, the purpose of calling getOptions() is to discover what options to use when creating the AST. But the act of calling getOptions(true) loads the IElementInfo, which then calls openWhenClosed(), and then calls buildStructure(). Almost all calls to buildStructure will, in fact, parse the file in one way or another.

Upon returning back up the stack, the parser in the snippet above will, also, parse the file.

The end result is the file gets parsed twice, and the CU gets opened. But the javadoc does not specify that as the result. The javadoc says it should "Returns the table of the current custom options for this ICompilationUnit" and if setOptions() hasn't been called, it should just return empty.

I think checking if the element info has been loaded should be sufficient. If it hasn't been created, nobody could have called setOptions() on it, and the empty map should be returned. This would avoid the first unnecessary parse as a side effect of loading the options.

I suspect, but am unable to prove conclusively, that this is a safe patch.

What it does

How to test

Author checklist

@robstryker
Copy link
Contributor Author

I'd like to note I have left the pre-existing code in place just so I can see how this PR performs against the test suite.

…nt info

Signed-off-by: Rob Stryker <[email protected]>

Cleanup

Signed-off-by: Rob Stryker <[email protected]>
@robstryker
Copy link
Contributor Author

Patch is easier to read if you ignore whitespace: https://github.com/eclipse-jdt/eclipse.jdt.core/pull/4779/changes?w=1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant