-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Consider catching Exception
instead of Throwable
#3826
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
Changes from all commits
05b96fc
8197458
64efcb3
347f24b
ae4f3a7
d741ceb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -35,18 +35,15 @@ public abstract class DOMDeserializer<T> extends FromStringDeserializer<T> | |
parserFactory.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true); | ||
} catch(ParserConfigurationException pce) { | ||
// not much point to do anything; could log but... | ||
} catch (Error e) { | ||
// 14-Jul-2016, tatu: Not sure how or why, but during code coverage runs | ||
// (via Cobertura) we get `java.lang.AbstractMethodError` so... ignore that too | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was no trace of code coverage tool named Cobertura in databind. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why catch Exception though? Remove the Error catching but don't add the Exception handling. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed, let's just remove altogether in this case.But careful with following ones. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You mean, keep catching There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Keep all checks except for the Error check. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes. |
||
|
||
// [databind#2589] add two more settings just in case | ||
try { | ||
parserFactory.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true); | ||
} catch (Throwable t) { } // as per previous one, nothing much to do | ||
} catch (Exception t) { } // as per previous one, nothing much to do | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In this case need to leave (or could consider catching There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I do prefer There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Exactly. I do not (alas!) trust them NOT to throw, say, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Exception it is then~ |
||
try { | ||
parserFactory.setFeature("http://apache.org/xml/features/nonvalidating/load-external-dtd", false); | ||
} catch (Throwable t) { } // as per previous one, nothing much to do | ||
} catch (Exception t) { } // as per previous one, nothing much to do | ||
DEFAULT_PARSER_FACTORY = parserFactory; | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1300,10 +1300,10 @@ public static Method[] getClassMethods(Class<?> cls) | |
} | ||
try { | ||
return contextClass.getDeclaredMethods(); // Cross fingers | ||
} catch (Throwable t) { | ||
} catch (Exception t) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Makes sense, I think, can change to only catch |
||
return _failGetClassMethods(cls, t); | ||
} | ||
} catch (Throwable t) { | ||
} catch (Exception t) { | ||
return _failGetClassMethods(cls, t); | ||
} | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -58,7 +58,7 @@ public class StdDateFormat | |
+"(\\.\\d+)?" // optional second fractions | ||
+"(Z|[+-]\\d\\d(?:[:]?\\d\\d)?)?" // optional timeoffset/Z | ||
); | ||
} catch (Throwable t) { | ||
} catch (Exception t) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, makes sense. |
||
throw new RuntimeException(t); | ||
} | ||
PATTERN_ISO8601 = p; | ||
|
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.
Yes, these (in this class) make sense.