http://cr.openjdk.java.net/~henryjen/jdk9/8033716/1/webrev/src/share/classes/com/sun/imageio/plugins/gif/GIFImageReader.java.sdiff.html
230 public Iterator<ImageTypeSpecifier> getImageTypes(int imageIndex)
throws IIOException {
Nitpicky perhaps, but this looks > 80 chars. If so please split it.
-----
W.r.t the following change ...
http://cr.openjdk.java.net/~henryjen/jdk9/8033716/1/webrev/src/share/classes/com/sun/imageio/plugins/jpeg/DHTMarkerSegment.java.sdiff.html
145 class Htable implements Cloneable {
...
<208 protected Object clone() {
208 protected Htable clone()
---------
exactly what warning is this suppressing ?
Granted the language now allows returning a sub-class in over-riding,
but I don't
know why its a problem to return the declared type of the over-ridden
method?
The tool must have special knowledge of the semantics of the Cloneable
interface
to even call this out !
Same comment/question applies to DQTMarkerSegment.clone,
JFIFMarkerSegment.clone() and any other like that you might have changed ..
inc. the superclassMarkerSegment which I just spotted ... plus
SOFMarkerSegment,S
SOMarkerSegment
Maybe these will be OK but I need an explanation.
for (Iterator<JFIFExtensionMarkerSegment> iter = extSegments.iterator();
iter.hasNext();) {232 for (Iterator<JFIFExtensionMarkerSegment> iter =
extSegments.iterator(); iter.hasNext();) {
635 for (Iterator<JFIFExtensionMarkerSegment> iter =
extSegments.iterator(); iter.hasNext();)
More very long lines ...
I see yet more in JPEGMetadata : 663, 573, 688, 698, 710 .. too many more to
call out !
Finally, again note this change when approved should go into jdk9/client.
-phil.
On 2/14/2014 5:05 PM, Henry Jen wrote:
As there is no other comments so far, I posted updated version adapted
comments from Phil.
- removed public field comments from BMPMetadata
- removed not needed comments from GIFImageMetadata
http://cr.openjdk.java.net/~henryjen/jdk9/8033716/1/webrev/
Cheers,
Henry
On 02/07/2014 04:27 PM, Henry Jen wrote:
On 02/07/2014 03:00 PM, Phil Race wrote:
BMPMetadata.java
94 // Fields from CommentExtension
95 // List of byte[]
96 public List<byte[]> comments = null; // new ArrayList();
hmm .. how did you decide this was correct, other than trusting the
comment?
For this one, I took it from the comment, after verified similar types
in GIF.
The thing is I can't actually see where this field is used and I'm
inclined
to think this was a copy/paste from the GIF code.
It would seem the right thing to do is delete these lines.
It is public, so I don't dare to remove it, although it seems like
nothing in com.sun.imageio ever make use of this field.
Also, from the BMPMetadataFormat, CommentExtension is a string type, so
I am not really sure what's the best to do here.
If you think it is safe to remove it, I am more than happy to remove it.
In the case of GIFImageMetadata they are used but you left the comments
saying
// new ArrayList();
since it looks like you use the diamond operator now that should
not be completely true. Either remove the comment or, since it
seems it was intended to be informative update it to say
// new ArrayList<>();
Agree, I'll remove those comments.
I will have to look at all the subsequent files later ..
Thanks for reviewing it.
Cheers,
Henry
