On 02/19/2014 01:46 PM, Phil Race wrote:
.
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.
Will do a round to split lone lines to keep them under 80.
-----
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 ?
This eliminate "unchecked" cast warning when calling this method to get
an instance with correct class type.
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 !
I will change those if we wanted to stick to 80 column limit. I seems to
see it loosed to 132 column in many places. Personally I stick to 80 but
tolerate to 100 when I feel it's more readable.
Finally, again note this change when approved should go into jdk9/client.
I'll rebase the patch.
Cheers,
Henry
