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

review: fix: Source position of CtCatch and CtCatchVariable #2128

Merged
merged 5 commits into from
Jul 1, 2018

Conversation

pvojtechovsky
Copy link
Collaborator

@tdurieux do you agree that the test added by this PR should pass?

CtTry tryStatement = ...;
assertEquals("catch (IOException e) {\n" + 
		"			throw new RuntimeException(e);\n" + 
		"		}", contentAtPosition(classContent, tryStatement.getCatchers().get(0).getPosition()));

note: actually CtCatch source position contains only from parameters. E.g.: IOException e, which is not correct.

WDYT?

I will try to fix it in next days if you agree.

@tdurieux
Copy link
Collaborator

Yes, I agreed.

to get the catch position you should look at the jdt parent element.
I kind of did it for the catch variables

@pvojtechovsky pvojtechovsky changed the title test: CtCatch source position WIP: test: CtCatch source position Jun 28, 2018
@pvojtechovsky pvojtechovsky changed the title WIP: test: CtCatch source position fix: Source position of CtCatch and CtCatchVariable Jun 29, 2018
@tdurieux
Copy link
Collaborator

It seems good to me. Thanks @pvojtechovsky

@pvojtechovsky pvojtechovsky changed the title fix: Source position of CtCatch and CtCatchVariable review: fix: Source position of CtCatch and CtCatchVariable Jun 30, 2018
@pvojtechovsky
Copy link
Collaborator Author

I am done here.

}
int bracketStart = findNextNonWhitespace(contents, endOfTry, catchStart + CATCH.length());
if (bracketStart < 0) {
throw new SpoonException("Unexpected end of file instead of \'(\' after catch statement on offset: " + catchStart);
Copy link
Collaborator

@tdurieux tdurieux Jun 30, 2018

Choose a reason for hiding this comment

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

I'm afraid of this exception because there is a chance that we forget to handle one specific case and it will break the construction of the model.
I agree it is good to fix the problem but do you need to force this behavior to the users.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What do you suggest? To silently ignore our mistake ("we forget to handle one specific case") and to produce invalid source position?

Copy link
Collaborator

@tdurieux tdurieux Jun 30, 2018

Choose a reason for hiding this comment

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

I don't know what is the best solution.
I just thought you can use the environment enableCheck, this way the user can disable it and you still have a chance to have a feedback.
WDYT?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I meant skipSelfChecks

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ok, I will do so

@pvojtechovsky
Copy link
Collaborator Author

PositionBuilder now writes into log and returns SourcePosition.NOPOSITION when there is a position detection problem and model checks are disabled. If they are enabled then it throws exception on each such problem.

@monperrus monperrus merged commit bf66d6e into INRIA:master Jul 1, 2018
@monperrus
Copy link
Collaborator

thanks Pavel

@pvojtechovsky pvojtechovsky deleted the fixCatchPosition branch July 1, 2018 08:03
@surli surli mentioned this pull request Jul 4, 2018
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.

3 participants