Re: [OpenJDK 2D-Dev] JDK 9: RFR: 8033716: Fix raw and unchecked lint warnings in com.sun.imageio
On 2/20/2014 12:05 AM, Henry Jen wrote:
On 02/19/2014 02:59 PM, Henry Jen wrote:
On 02/19/2014 01:46 PM, Phil Race wrote:
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.
That's not exactly correct, it just eliminate the need to cast, not
warning.
Would you advice to revert them or keep it?
In that case, I'd suggest to revert them
-phil.
Re: [OpenJDK 2D-Dev] JDK 9: RFR: 8033716: Fix raw and unchecked lint warnings in com.sun.imageio
Hello Henry,
please see my comments regrading the fallthrough warnings inline.
On 2/20/2014 12:05 PM, Henry Jen wrote:
On 02/19/2014 02:59 PM, Henry Jen wrote:
On 02/19/2014 01:46 PM, Phil Race wrote:
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.
That's not exactly correct, it just eliminate the need to cast, not
warning.
Would you advice to revert them or keep it?
Also, I noted there are a couple fallthrough warnings, the
BMPImageReader one seems valid and should be fixed while the others
looks correct, but I am not exactly sure about the image format, so I
think I'll just list it here and perhaps someone with expertise can
review them?
/home/hjen/ws/9-client/jdk/src/share/classes/com/sun/imageio/plugins/bmp/BMPImageReader.java:916:
warning: [fallthrough] possible fall-through into case
case VERSION_4_8_BIT:
^
This warning needs to be fixed: the case VERSION_4_4_BIT does not have
the break operator.
/home/hjen/ws/9-client/jdk/src/share/classes/com/sun/imageio/plugins/jpeg/JPEGImageReader.java:429:
warning: [fallthrough] possible fall-through into case
case 0: // not a marker, just a data 0xff
^
This case is correct, the warning probably should be suppressed.
/home/hjen/ws/9-client/jdk/src/share/classes/com/sun/imageio/plugins/jpeg/JPEGImageWriter.java:1554:
warning: [fallthrough] possible fall-through into case
case ColorSpace.TYPE_CMYK:
^
This warning needs to be fixed: the case ColorSpace.TYPE_3CLR missed the
break operator.
/home/hjen/ws/9-client/jdk/src/share/classes/com/sun/imageio/plugins/jpeg/JPEGImageWriter.java:1593:
warning: [fallthrough] possible fall-through into case
case ColorSpace.TYPE_CMYK:
^
This warning needs to be fixed: the case ColorSpace.TYPE_3CLR missed the
break operator.
/home/hjen/ws/9-client/jdk/src/share/classes/com/sun/imageio/plugins/jpeg/JPEGImageWriter.java:1639:
warning: [fallthrough] possible fall-through into case
case ColorSpace.TYPE_CMYK:
This warning needs to be fixed: the case ColorSpace.TYPE_3CLR missed the
break operator.
Thanks,
Andrew
Cheers,
Henry
Re: [OpenJDK 2D-Dev] JDK 9: RFR: 8033716: Fix raw and unchecked lint warnings in com.sun.imageio
On 02/19/2014 02:59 PM, Henry Jen wrote:
On 02/19/2014 01:46 PM, Phil Race wrote:
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.
That's not exactly correct, it just eliminate the need to cast, not warning.
Would you advice to revert them or keep it?
Also, I noted there are a couple fallthrough warnings, the
BMPImageReader one seems valid and should be fixed while the others
looks correct, but I am not exactly sure about the image format, so I
think I'll just list it here and perhaps someone with expertise can
review them?
/home/hjen/ws/9-client/jdk/src/share/classes/com/sun/imageio/plugins/bmp/BMPImageReader.java:916:
warning: [fallthrough] possible fall-through into case
case VERSION_4_8_BIT:
^
/home/hjen/ws/9-client/jdk/src/share/classes/com/sun/imageio/plugins/jpeg/JPEGImageReader.java:429:
warning: [fallthrough] possible fall-through into case
case 0: // not a marker, just a data 0xff
^
/home/hjen/ws/9-client/jdk/src/share/classes/com/sun/imageio/plugins/jpeg/JPEGImageWriter.java:1554:
warning: [fallthrough] possible fall-through into case
case ColorSpace.TYPE_CMYK:
^
/home/hjen/ws/9-client/jdk/src/share/classes/com/sun/imageio/plugins/jpeg/JPEGImageWriter.java:1593:
warning: [fallthrough] possible fall-through into case
case ColorSpace.TYPE_CMYK:
^
/home/hjen/ws/9-client/jdk/src/share/classes/com/sun/imageio/plugins/jpeg/JPEGImageWriter.java:1639:
warning: [fallthrough] possible fall-through into case
case ColorSpace.TYPE_CMYK:
Cheers,
Henry
Re: [OpenJDK 2D-Dev] JDK 9: RFR: 8033716: Fix raw and unchecked lint warnings in com.sun.imageio
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 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 iter =
extSegments.iterator(); iter.hasNext();) {232 for
(Iterator iter = extSegments.iterator();
iter.hasNext();) {
635 for (Iterator 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
Re: [OpenJDK 2D-Dev] JDK 9: RFR: 8033716: Fix raw and unchecked lint warnings in com.sun.imageio
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 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 iter = extSegments.iterator();
iter.hasNext();) {232 for (Iterator iter =
extSegments.iterator(); iter.hasNext();) {
635 for (Iterator 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 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
Re: [OpenJDK 2D-Dev] JDK 9: RFR: 8033716: Fix raw and unchecked lint warnings in com.sun.imageio
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 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
Re: [OpenJDK 2D-Dev] JDK 9: RFR: 8033716: Fix raw and unchecked lint warnings in com.sun.imageio
On 02/07/2014 03:00 PM, Phil Race wrote: BMPMetadata.java 94 // Fields from CommentExtension 95 // List of byte[] 96 public List 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
Re: [OpenJDK 2D-Dev] JDK 9: RFR: 8033716: Fix raw and unchecked lint warnings in com.sun.imageio
On 2/7/2014 2:20 PM, Phil Race wrote: Yes, it should get 2d review and I will look at this soon as priorities permit but the *conclusion* is that the client team ask that such changes go into the client forest. If this is a problem for you then we will do it on your behalf. We do not want client changes directly into dev. That is a very polite but firm request. If such changes are going to go into the client forest rather than dev, there has to be some bounded timeline for the fixes to get integrated from client into dev. The client and dev forests were created back in December 2013. Since then, there have been two promoted builds of JDK 9. The client forest has not integrated yet into dev; in contrast, the HotSpot forest has integrated into dev at least twice. An implication of using a separate client forest for client library fixes is that the forest has to be actively maintained, both syncing down changes from dev regularly and integrating changes into dev regularly. -Joe -phil. On 2/7/2014 1:54 PM, Henry Jen wrote: Thanks Joe for reviewing. I would like to get 2d developer review as well before pushing this, let me know if that's not necessary. Also there was a discussion ealier on whether such change should go to client or jdk9/dev repo, do we have a conclusion? Cheers, Henry On 02/05/2014 06:01 PM, Joe Darcy wrote: Hi Henry, On 02/05/2014 12:19 PM, Henry Jen wrote: Hi, Please review the webrev to clean up raw and unchecked warnings in com.sun.imageio packag at, http://cr.openjdk.java.net/~henryjen/jdk9/8033716/0/webrev/ The more significant change in this webrev is that I have changed the clone() method of MarkerSegment-derived classes to return exact type rather than Object. Otherwise, it's basically add type information and eliminate cast no longer needed. Cheers, Henry I looked over the changes and they generally look good. I've taken a closer look at the clone-related changes. The types SOSMarkerSegment, SOFMarkerSegment, MarkerSegment, etc. are call package-private and all have had their Object-returning clone methods replaced with a self-type returning clone method, a covariant override. Since there are no other potential subclasses of these com.sun.* type that would already have had a covariant override of clone, I believe the changes to clone on these three methods is fine. (This avoid the hazards outlined in JDK-7140820: (coll) Add covariant overrides to Collections clone methods.) Thanks, -Joe
Re: [OpenJDK 2D-Dev] JDK 9: RFR: 8033716: Fix raw and unchecked lint warnings in com.sun.imageio
BMPMetadata.java 94 // Fields from CommentExtension 95 // List of byte[] 96 public List comments = null; // new ArrayList(); hmm .. how did you decide this was correct, other than trusting the comment? 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. 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<>(); I will have to look at all the subsequent files later .. -phil. On 2/7/2014 1:54 PM, Henry Jen wrote: Thanks Joe for reviewing. I would like to get 2d developer review as well before pushing this, let me know if that's not necessary. Also there was a discussion ealier on whether such change should go to client or jdk9/dev repo, do we have a conclusion? Cheers, Henry On 02/05/2014 06:01 PM, Joe Darcy wrote: Hi Henry, On 02/05/2014 12:19 PM, Henry Jen wrote: Hi, Please review the webrev to clean up raw and unchecked warnings in com.sun.imageio packag at, http://cr.openjdk.java.net/~henryjen/jdk9/8033716/0/webrev/ The more significant change in this webrev is that I have changed the clone() method of MarkerSegment-derived classes to return exact type rather than Object. Otherwise, it's basically add type information and eliminate cast no longer needed. Cheers, Henry I looked over the changes and they generally look good. I've taken a closer look at the clone-related changes. The types SOSMarkerSegment, SOFMarkerSegment, MarkerSegment, etc. are call package-private and all have had their Object-returning clone methods replaced with a self-type returning clone method, a covariant override. Since there are no other potential subclasses of these com.sun.* type that would already have had a covariant override of clone, I believe the changes to clone on these three methods is fine. (This avoid the hazards outlined in JDK-7140820: (coll) Add covariant overrides to Collections clone methods.) Thanks, -Joe
Re: [OpenJDK 2D-Dev] JDK 9: RFR: 8033716: Fix raw and unchecked lint warnings in com.sun.imageio
Yes, it should get 2d review and I will look at this soon as priorities permit but the *conclusion* is that the client team ask that such changes go into the client forest. If this is a problem for you then we will do it on your behalf. We do not want client changes directly into dev. That is a very polite but firm request. -phil. On 2/7/2014 1:54 PM, Henry Jen wrote: Thanks Joe for reviewing. I would like to get 2d developer review as well before pushing this, let me know if that's not necessary. Also there was a discussion ealier on whether such change should go to client or jdk9/dev repo, do we have a conclusion? Cheers, Henry On 02/05/2014 06:01 PM, Joe Darcy wrote: Hi Henry, On 02/05/2014 12:19 PM, Henry Jen wrote: Hi, Please review the webrev to clean up raw and unchecked warnings in com.sun.imageio packag at, http://cr.openjdk.java.net/~henryjen/jdk9/8033716/0/webrev/ The more significant change in this webrev is that I have changed the clone() method of MarkerSegment-derived classes to return exact type rather than Object. Otherwise, it's basically add type information and eliminate cast no longer needed. Cheers, Henry I looked over the changes and they generally look good. I've taken a closer look at the clone-related changes. The types SOSMarkerSegment, SOFMarkerSegment, MarkerSegment, etc. are call package-private and all have had their Object-returning clone methods replaced with a self-type returning clone method, a covariant override. Since there are no other potential subclasses of these com.sun.* type that would already have had a covariant override of clone, I believe the changes to clone on these three methods is fine. (This avoid the hazards outlined in JDK-7140820: (coll) Add covariant overrides to Collections clone methods.) Thanks, -Joe
Re: [OpenJDK 2D-Dev] JDK 9: RFR: 8033716: Fix raw and unchecked lint warnings in com.sun.imageio
Thanks Joe for reviewing. I would like to get 2d developer review as well before pushing this, let me know if that's not necessary. Also there was a discussion ealier on whether such change should go to client or jdk9/dev repo, do we have a conclusion? Cheers, Henry On 02/05/2014 06:01 PM, Joe Darcy wrote: Hi Henry, On 02/05/2014 12:19 PM, Henry Jen wrote: Hi, Please review the webrev to clean up raw and unchecked warnings in com.sun.imageio packag at, http://cr.openjdk.java.net/~henryjen/jdk9/8033716/0/webrev/ The more significant change in this webrev is that I have changed the clone() method of MarkerSegment-derived classes to return exact type rather than Object. Otherwise, it's basically add type information and eliminate cast no longer needed. Cheers, Henry I looked over the changes and they generally look good. I've taken a closer look at the clone-related changes. The types SOSMarkerSegment, SOFMarkerSegment, MarkerSegment, etc. are call package-private and all have had their Object-returning clone methods replaced with a self-type returning clone method, a covariant override. Since there are no other potential subclasses of these com.sun.* type that would already have had a covariant override of clone, I believe the changes to clone on these three methods is fine. (This avoid the hazards outlined in JDK-7140820: (coll) Add covariant overrides to Collections clone methods.) Thanks, -Joe
Re: [OpenJDK 2D-Dev] JDK 9: RFR: 8033716: Fix raw and unchecked lint warnings in com.sun.imageio
Hi Henry, On 02/05/2014 12:19 PM, Henry Jen wrote: Hi, Please review the webrev to clean up raw and unchecked warnings in com.sun.imageio packag at, http://cr.openjdk.java.net/~henryjen/jdk9/8033716/0/webrev/ The more significant change in this webrev is that I have changed the clone() method of MarkerSegment-derived classes to return exact type rather than Object. Otherwise, it's basically add type information and eliminate cast no longer needed. Cheers, Henry I looked over the changes and they generally look good. I've taken a closer look at the clone-related changes. The types SOSMarkerSegment, SOFMarkerSegment, MarkerSegment, etc. are call package-private and all have had their Object-returning clone methods replaced with a self-type returning clone method, a covariant override. Since there are no other potential subclasses of these com.sun.* type that would already have had a covariant override of clone, I believe the changes to clone on these three methods is fine. (This avoid the hazards outlined in JDK-7140820: (coll) Add covariant overrides to Collections clone methods.) Thanks, -Joe
[OpenJDK 2D-Dev] JDK 9: RFR: 8033716: Fix raw and unchecked lint warnings in com.sun.imageio
Hi, Please review the webrev to clean up raw and unchecked warnings in com.sun.imageio packag at, http://cr.openjdk.java.net/~henryjen/jdk9/8033716/0/webrev/ The more significant change in this webrev is that I have changed the clone() method of MarkerSegment-derived classes to return exact type rather than Object. Otherwise, it's basically add type information and eliminate cast no longer needed. Cheers, Henry
