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: fix regression #2846 #2842 #2848

Merged
merged 1 commit into from
Jan 8, 2019
Merged

Conversation

monperrus
Copy link
Collaborator

fix #2846
fix #2842

@monperrus
Copy link
Collaborator Author

ready.

@@ -539,9 +538,6 @@ private void setModifiersPosition(CtModifiable e, int start, int end) {
}
start = o2;
}
if (explicitModifiersByName.size() > 0) {
throw new SpoonException("Position of CtExtendedModifiers: [" + String.join(", ", explicitModifiersByName.keySet()) + "] not found in " + String.valueOf(contents, start, end - start));
}
Copy link
Collaborator

@nharrand nharrand Jan 8, 2019

Choose a reason for hiding this comment

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

Is there a legitimate reason why final modifier would remain in explicitModifiersByName here?

To be more clear: If I understand correctly, in some/all cases, final modifiers are left in explicitModifiersByName and therefor trigger an exception. Am I correct? And if yes, is it a false negative (a wrongfully thrown exception) or are you just allowing AST build to pass with slightly incomplete position information?

@monperrus
Copy link
Collaborator Author

monperrus commented Jan 8, 2019 via email

@nharrand
Copy link
Collaborator

nharrand commented Jan 8, 2019

OK. Works for me!

@nharrand nharrand merged commit 8b5f0c5 into INRIA:master Jan 8, 2019
@tdurieux
Copy link
Collaborator

tdurieux commented Jan 9, 2019

This PR was not a fix, it was a workaround. There is indeed a bug in the implementation of positionbuilder and this PR will complexify the implementation of the sniper mode.

The bug was that there was no whitespace before or after the modifier and consequently the positionBuilder did not find it and trigger an exception.

@nharrand
Copy link
Collaborator

nharrand commented Jan 9, 2019

@tdurieux would you be more comfortable with #2854 ?

@monperrus
Copy link
Collaborator Author

#2854 is good, thanks.

Yet, there is a philosophical argument here:

  • are we able to have a working solution for 100% of cases? can we predict all cases?
  • if one case is missing, should we break the execution by throwing an exception?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug when parsing 'final' regression in gumtree-spoon-ast-diff
3 participants