Navigation Menu

Skip to content
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

fix: OOB in JDTCommentBuilder #2840

Merged
merged 4 commits into from Dec 18, 2018
Merged

Conversation

Egor18
Copy link
Contributor

@Egor18 Egor18 commented Dec 12, 2018

I think that refactoring of JDTCommentBuilder in #2822 did not go so well :|
This PR is to fix IndexOutOfBoundsException during model build for the following code:

void bug3() {
    if (false) {} // some comment
    else if (false) {}
}
java.lang.IndexOutOfBoundsException: Index: 0

	at spoon.support.util.EmptyClearableList.get(EmptyClearableList.java:90)
	at spoon.support.util.ModelList.get(ModelList.java:55)
	at spoon.support.reflect.code.CtBlockImpl.getStatement(CtBlockImpl.java:78)
	at spoon.support.compiler.jdt.JDTCommentBuilder$1.visitCtIf(JDTCommentBuilder.java:378)
	at spoon.support.reflect.code.CtIfImpl.accept(CtIfImpl.java:45)
	at spoon.reflect.visitor.CtInheritanceScanner.scan(CtInheritanceScanner.java:184)
	at spoon.support.compiler.jdt.JDTCommentBuilder$1.scan(JDTCommentBuilder.java:233)
	at spoon.support.compiler.jdt.JDTCommentBuilder.insertCommentInAST(JDTCommentBuilder.java:437)
	at spoon.support.compiler.jdt.JDTCommentBuilder.buildComment(JDTCommentBuilder.java:147)

@monperrus
Copy link
Collaborator

LGTM. @pvojtechovsky do you merge?

SourcePosition elsePosition = elseStatement.getPosition().isValidPosition() ? elseStatement.getPosition() : elseExpression.getPosition();
SourcePosition thenPosition = thenStatement.getPosition();
if (!thenPosition.isValidPosition()) {
CtStatement thenExpression = ((CtBlock) thenStatement).getStatement(0);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thenStatement isn't always CtBlock in this context.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I see, but it's exactly the way the original code was written (I did not really want to change it):

SourcePosition thenPosition = e.getThenStatement().getPosition().isValidPosition() == false ? ((CtBlock) e.getThenStatement()).getStatement(0).getPosition() : e.getThenStatement().getPosition();
SourcePosition elsePosition = e.getElseStatement().getPosition().isValidPosition() == false ? ((CtBlock) e.getElseStatement()).getStatement(0).getPosition() : e.getElseStatement().getPosition();

Would you like me to add an additional check like thenStatement instanceof CtBlock?

@pvojtechovsky
Copy link
Collaborator

I see that it is same code like before. I suggest to write it correctly, when we are already touching it.

Would you like me to add an additional check like thenStatement instanceof CtBlock?

yes please

@pvojtechovsky pvojtechovsky merged commit 4048686 into INRIA:master Dec 18, 2018
@pvojtechovsky
Copy link
Collaborator

Thank you @Egor18

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.

None yet

3 participants