-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
8270058: Use Objects.check{Index,FromIndexSize} for java.desktop #4718
Changes from 3 commits
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 |
---|---|---|
|
@@ -32,6 +32,7 @@ | |
import java.io.ByteArrayInputStream; | ||
import java.io.IOException; | ||
import java.util.Iterator; | ||
import java.util.Objects; | ||
import java.util.Vector; | ||
|
||
import javax.imageio.IIOImage; | ||
|
@@ -87,8 +88,7 @@ public BufferedImage read(int imageIndex, ImageReadParam param) | |
throws IOException { | ||
if (input == null) | ||
throw new IllegalStateException(); | ||
if (imageIndex >= 1 || imageIndex < 0) | ||
throw new IndexOutOfBoundsException(); | ||
Objects.checkIndex(imageIndex, 1); | ||
if (seekForwardOnly) { | ||
if (imageIndex < minIndex) | ||
throw new IndexOutOfBoundsException(); | ||
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. Can't we use checkFromToIndex for this IOOBE too? Used in many other places in this PR... Please also confirm all tests modified are green after this modification.. 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. checkFromToIndex has different semantice with checkIndex:
The following tests are passed after applying this patch:
ClippedImages.java is a manual test, I have no environment to test it, but since it's a trivial replacement, I think it's okay.. 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 meant
can't it be replaced with Objects.checkFromToIndex(minIndex, imageIndex, .. 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. Hi, sorry for the late response. checkFromToIndex has many constraints, this code may not satisfy those constraints, so I don't think we should do that in this PR.(Maybe we can file a new PR to do replacement carefully) 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. OK...The subject line of this PR states that checkFromToIndex will also be used but I see only checkIndex so I intervened. Anyways, the code in question is in test so not much of an issue. 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. Thanks @prsadhuk for review! I've updated the title to exclude checkFromToIndex. |
||
|
@@ -101,8 +101,7 @@ public BufferedImage read(int imageIndex, ImageReadParam param) | |
public Iterator getImageTypes(int imageIndex) throws IOException { | ||
if (input == null) | ||
throw new IllegalStateException(); | ||
if (imageIndex >= 1 || imageIndex < 0) | ||
throw new IndexOutOfBoundsException(); | ||
Objects.checkIndex(imageIndex, 1); | ||
|
||
Vector imageTypes = new Vector(); | ||
imageTypes.add(ImageTypeSpecifier.createFromBufferedImageType | ||
|
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 we should not replace IOOBE checks with messages. Its better to keep it old way.
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.
Objects.checkIndex itself throws exceptions for wrong values. So this should be ok. No?
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.
We have specific message here like "Tab count" which we are not passing and will be helpful for debugging in future.
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.
Restored.