-
Notifications
You must be signed in to change notification settings - Fork 319
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
Changed AbstractAntlrParserAdapter to catch all exceptions during par… #1480
Conversation
@@ -69,7 +68,7 @@ private void parseFile(File file, TokenCollector collector) throws ParsingExcept | |||
for (ParseTree child : entryContext.children) { | |||
treeWalker.walk(listener, child); | |||
} | |||
} catch (IOException exception) { | |||
} catch (Throwable exception) { // catching throwable to capture any exceptions thrown by ANTLR. |
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.
Isn't Exception enough ?
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.
It would probably be enough, but I think it could also help printing the file in case on OutOfMemoryError or similar things, especially since the original exception will still be printed as the cause.
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.
I also had this kind of Errors in mind. Nevertheless, I won't catch this kind of Error since this kind of errors can't be handled in a good way. Catching a OutOfMemoryError won't solve the problem as far as I remember :)
But I don't have a strong opinion on what to catch here :)
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.
This catch does not really handle the error, but instead adds some additional information for the user, so I would consider it a form of logging. The intention is not to continue if anything happens here. Just for reference I have tried this with a small example:
import java.util.ArrayList;
import java.util.List;
public class OutOfMemoryTest {
public static void main(String[] args) {
try {
List<int[]> items = new ArrayList<>();
for (int i = 0; i < Integer.MAX_VALUE; i++) {
items.add(new int[Integer.MAX_VALUE]);
}
} catch (Throwable e) {
throw new RuntimeException("Some error happened.", e);
}
}
}
If you compile and run this code the following error will be printed:
Exception in thread "main" java.lang.RuntimeException: Some error happened.
at OutOfMemoryTest.main(OutOfMemoryTest.java:12)
Caused by: java.lang.OutOfMemoryError: Requested array size exceeds VM limit
at OutOfMemoryTest.main(OutOfMemoryTest.java:9)
In our case, this would give the user the information which source file caused the error to happen. In the end I would leave the decision up to @tsaglam.
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.
One is better for API users, the other for application users. Let us go with Exception
for now, we can always change it.
This makes it, so all antlr exceptions are caught, so that the name of the file, that failed is always shown.
Addresses: #1386