-
Notifications
You must be signed in to change notification settings - Fork 241
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
Formats-BSD: Initial warnings cleanup #2766
Conversation
Variable success was not being used and the returned value overallSuccess was always true
Two general comments:
|
This reverts commit 6b52ad1.
Agreed that modifying the public signatures for the generics is probably in need of more thought and discussion rather than the intention of this PR which is to remove some basic warnings. Reverting the commit for those cases. |
@@ -152,6 +152,7 @@ public void makeOmeTiff(final String name, final CoreMetadata info) | |||
final String id = getId(name); | |||
final OMETiffWriter out = createWriter(name, info, id); | |||
writeData(name, info, id, out); | |||
out.close(); |
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.
All looks good and included in the last round of merge builds successfully. @joshmoore: are you happy with the latest state of this PR? |
out.close(); | ||
} | ||
catch (IOException e) { | ||
throw new FormatException("Could not compress JPEG-2000 data.", e); |
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 exception seems counter-intuitive.
One minor change request. The non- |
Did closing the MCIIS instances causes a problem? Sorry if I caused confusion: I didn't mean that the close was bad, just that it was unfortunate we couldn't use the new resource closing that we've used elsewhere. |
Ah that was just a misunderstanding on my part then. There were no issues from closing the MCIIS instances as far as I'm aware. I shall re-include them. |
95273e6
to
6fb0c12
Compare
All looks reasonable to me. Jobs are are green and seems fine to merge. |
This is part of the work to review the resources of various components for potential resource leaks.
As part of this I have completed an initial pass of the formats-bsd component and reviewed any warnings.
This PR cleans up some simple warnings throughout covering the following:
The one exception is the last commit 1d2c438 which also provides a small fix to upgradeChecker as a success check was not being used previously.
This is by no means an extensive or complete cleanup of warnings, simply some initial cleanup which would be easy and safe while identifying potential resource leaks.
To test:
All builds and tests should remain green