-
Notifications
You must be signed in to change notification settings - Fork 594
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 Java 11 deprecation warnings #6145
Conversation
#6119 has these changes (and more), but this PR has a set of simple changes that should be easy to merge. |
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.
@tomwhite I think we can avoid most of the suppressions because they're mostly Class.newInstance which has a replacement in java 8. I thought some of the new Integer
calls might be deliberate, but none of them look meaningful on further inspection. Most of them can be just removed entirely though since the compiler will box them if necessary.
@@ -298,6 +298,7 @@ private static boolean printStackTraceOnUserExceptions() { | |||
/** | |||
* Returns the command line program specified, or prints the usage and exits with exit code 1 * | |||
*/ | |||
@SuppressWarnings("deprecation") |
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 think most of the deprecation sites can be fixed by replace class.newInstance()
with class.getDeclaredConstructor().newInstance()
which seems to exist in 8. It means adding additional exceptions to the list of exceptions caught or thrown but I think that's preferable to keeping the deprecated method and suppressing it.
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.
{"-27", new Integer(-27)}, | ||
{"0", new Integer(0)}, | ||
{"-27014", new Integer(-27014)}, | ||
{"27", Integer.valueOf(27)}, |
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 was thinking this might have been a deliberate choice, but looking at it closely I don't think so. This seems fine.
} | ||
else if ( annotationsToCheckMaf.get(i).equals(MafOutputRendererConstants.FieldName_End_Position) ) { | ||
mafFieldValues = maf.getRecords().stream().map(AnnotatedInterval::getEnd).map(x -> new Integer(x)).map(Object::toString).collect(Collectors.toList()); | ||
mafFieldValues = maf.getRecords().stream().map(AnnotatedInterval::getEnd).map(x -> Integer.valueOf(x)).map(Object::toString).collect(Collectors.toList()); |
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 think these are unnecessary boxing and could just be removed.
score.addTotalReads(new Integer(tok[7])); | ||
score.addUnambiguousReads(new Integer(tok[8])); | ||
score.setReferenceLength(new Long(tok[9])); | ||
score.addSelfScore(Double.valueOf(tok[5])); |
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 should really be Double.parseDouble
instead but it's just a test so it doesn't really matter
@@ -104,7 +104,7 @@ public void testRandomMaskedKmers() { | |||
//Generate random indices to mask | |||
List<Byte> maskIndices = new ArrayList<>(K); | |||
for (int j = 0; j < K; j++) { | |||
maskIndices.add(new Byte((byte) j)); | |||
maskIndices.add(Byte.valueOf((byte) j)); |
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 can just be autoboxed
@@ -132,7 +132,7 @@ void resizeTest() { | |||
Assert.assertTrue(hopscotchSet.size() == hashSet.size()); | |||
LongIterator itrL = hopscotchSet.iterator(); | |||
while (itrL.hasNext()) { | |||
Assert.assertTrue(hashSet.contains(new Long(itrL.next()))); | |||
Assert.assertTrue(hashSet.contains(Long.valueOf(itrL.next()))); |
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.
likewise
1f5db31
to
3e30784
Compare
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.
👍 Looks good. Merge when tests pass.
3e30784
to
2e74072
Compare
Pre-req to testing on Java 11