RFR (JDK10/JAXP) 8181155: Fix lint warnings in JAXP repo: fallthrough and static
Hi, Please review a cleanup of fallthrough and static warnings. For fallthrough, the majority of the changes are suppressing the warnings, while for static, replacing the instances with the class. All jaxp tests and JCK passed. JBS: https://bugs.openjdk.java.net/browse/JDK-8181155 webrevs: http://cr.openjdk.java.net/~joehw/jdk10/8181155/webrev/index.html Thanks, Joe
Hi Joe The changes look OK Best Lance
On Oct 30, 2017, at 1:03 PM, Joe Wang <huizhe.wang@oracle.com> wrote:
Hi,
Please review a cleanup of fallthrough and static warnings. For fallthrough, the majority of the changes are suppressing the warnings, while for static, replacing the instances with the class.
All jaxp tests and JCK passed.
JBS: https://bugs.openjdk.java.net/browse/JDK-8181155 webrevs: http://cr.openjdk.java.net/~joehw/jdk10/8181155/webrev/index.html
Thanks, Joe
<http://oracle.com/us/design/oracle-email-sig-198324.gif> <http://oracle.com/us/design/oracle-email-sig-198324.gif> <http://oracle.com/us/design/oracle-email-sig-198324.gif> <http://oracle.com/us/design/oracle-email-sig-198324.gif>Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037 Oracle Java Engineering 1 Network Drive Burlington, MA 01803 Lance.Andersen@oracle.com <mailto:Lance.Andersen@oracle.com>
Thanks Lance! Best, Joe On 10/30/17, 10:09 AM, Lance Andersen wrote:
Hi Joe
The changes look OK
Best Lance
On Oct 30, 2017, at 1:03 PM, Joe Wang <huizhe.wang@oracle.com <mailto:huizhe.wang@oracle.com>> wrote:
Hi,
Please review a cleanup of fallthrough and static warnings. For fallthrough, the majority of the changes are suppressing the warnings, while for static, replacing the instances with the class.
All jaxp tests and JCK passed.
JBS: https://bugs.openjdk.java.net/browse/JDK-8181155 webrevs: http://cr.openjdk.java.net/~joehw/jdk10/8181155/webrev/index.html <http://cr.openjdk.java.net/%7Ejoehw/jdk10/8181155/webrev/index.html>
Thanks, Joe
<http://oracle.com/us/design/oracle-email-sig-198324.gif> <http://oracle.com/us/design/oracle-email-sig-198324.gif><http://oracle.com/us/design/oracle-email-sig-198324.gif> <http://oracle.com/us/design/oracle-email-sig-198324.gif>Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037 Oracle Java Engineering 1 Network Drive Burlington, MA 01803 Lance.Andersen@oracle.com <mailto:Lance.Andersen@oracle.com>
Hi Joe, +1 Is there a useful comment on the @SuppressWarnings like in other files: XSDHandler.java: 3028 DTMDocumentImpl: 1700 DOM2DTM.java: 1654 FilterExprWalker.java: 65 A few of the added breaks would have been hiding bugs. It might be worth mentioning them in the issue. Roger On 10/30/2017 1:09 PM, Lance Andersen wrote:
Hi Joe
The changes look OK
Best Lance
On Oct 30, 2017, at 1:03 PM, Joe Wang <huizhe.wang@oracle.com> wrote:
Hi,
Please review a cleanup of fallthrough and static warnings. For fallthrough, the majority of the changes are suppressing the warnings, while for static, replacing the instances with the class.
All jaxp tests and JCK passed.
JBS: https://bugs.openjdk.java.net/browse/JDK-8181155 webrevs: http://cr.openjdk.java.net/~joehw/jdk10/8181155/webrev/index.html
Thanks, Joe <http://oracle.com/us/design/oracle-email-sig-198324.gif> <http://oracle.com/us/design/oracle-email-sig-198324.gif> <http://oracle.com/us/design/oracle-email-sig-198324.gif> <http://oracle.com/us/design/oracle-email-sig-198324.gif>Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037 Oracle Java Engineering 1 Network Drive Burlington, MA 01803 Lance.Andersen@oracle.com <mailto:Lance.Andersen@oracle.com>
On 10/30/17, 11:14 AM, Roger Riggs wrote:
Hi Joe,
+1
Is there a useful comment on the @SuppressWarnings like in other files:
XSDHandler.java: 3028 DTMDocumentImpl: 1700 DOM2DTM.java: 1654 FilterExprWalker.java: 65
For the methods with long or very long switch statements, I added a note following the SuppressWarnings annotation to indicate where fallthrough would happen and in which case warnings were suppressed. But for the pretty short ones like the above, I thought it's quite obvious where fallthrough might happen, I didn't therefore add any comment.
A few of the added breaks would have been hiding bugs. It might be worth mentioning them in the issue.
I added a note to the issue. Thanks, Joe
Roger
On 10/30/2017 1:09 PM, Lance Andersen wrote:
Hi Joe
The changes look OK
Best Lance
On Oct 30, 2017, at 1:03 PM, Joe Wang <huizhe.wang@oracle.com> wrote:
Hi,
Please review a cleanup of fallthrough and static warnings. For fallthrough, the majority of the changes are suppressing the warnings, while for static, replacing the instances with the class.
All jaxp tests and JCK passed.
JBS: https://bugs.openjdk.java.net/browse/JDK-8181155 webrevs: http://cr.openjdk.java.net/~joehw/jdk10/8181155/webrev/index.html
Thanks, Joe <http://oracle.com/us/design/oracle-email-sig-198324.gif> <http://oracle.com/us/design/oracle-email-sig-198324.gif> <http://oracle.com/us/design/oracle-email-sig-198324.gif> <http://oracle.com/us/design/oracle-email-sig-198324.gif>Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037 Oracle Java Engineering 1 Network Drive Burlington, MA 01803 Lance.Andersen@oracle.com <mailto:Lance.Andersen@oracle.com>
Hi Joe, As an alternative, consider documenting the semantics/correctness of the fall-through at the location of the fall-through rather than at the top of the switch or method. Thanks, -Joe On 10/30/2017 12:17 PM, Joe Wang wrote:
On 10/30/17, 11:14 AM, Roger Riggs wrote:
Hi Joe,
+1
Is there a useful comment on the @SuppressWarnings like in other files:
XSDHandler.java: 3028 DTMDocumentImpl: 1700 DOM2DTM.java: 1654 FilterExprWalker.java: 65
For the methods with long or very long switch statements, I added a note following the SuppressWarnings annotation to indicate where fallthrough would happen and in which case warnings were suppressed. But for the pretty short ones like the above, I thought it's quite obvious where fallthrough might happen, I didn't therefore add any comment.
A few of the added breaks would have been hiding bugs. It might be worth mentioning them in the issue.
I added a note to the issue.
Thanks, Joe
Roger
On 10/30/2017 1:09 PM, Lance Andersen wrote:
Hi Joe
The changes look OK
Best Lance
On Oct 30, 2017, at 1:03 PM, Joe Wang <huizhe.wang@oracle.com> wrote:
Hi,
Please review a cleanup of fallthrough and static warnings. For fallthrough, the majority of the changes are suppressing the warnings, while for static, replacing the instances with the class.
All jaxp tests and JCK passed.
JBS: https://bugs.openjdk.java.net/browse/JDK-8181155 webrevs: http://cr.openjdk.java.net/~joehw/jdk10/8181155/webrev/index.html
Thanks, Joe <http://oracle.com/us/design/oracle-email-sig-198324.gif> <http://oracle.com/us/design/oracle-email-sig-198324.gif> <http://oracle.com/us/design/oracle-email-sig-198324.gif> <http://oracle.com/us/design/oracle-email-sig-198324.gif>Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037 Oracle Java Engineering 1 Network Drive Burlington, MA 01803 Lance.Andersen@oracle.com <mailto:Lance.Andersen@oracle.com>
On 10/30/17, 12:22 PM, joe darcy wrote:
Hi Joe,
As an alternative, consider documenting the semantics/correctness of the fall-through at the location of the fall-through rather than at the top of the switch or method.
Yes, in many of these cases, there already have been comments such as "fall through" at the location of the fall-through. But since some of the methods were very long, I thought it would make it easies for others who might read these method to understand by searching the keywords "case ...". The ones I added then served as an additional pointer to where fallthrough happens. Thanks, Joe
Thanks,
-Joe
On 10/30/2017 12:17 PM, Joe Wang wrote:
On 10/30/17, 11:14 AM, Roger Riggs wrote:
Hi Joe,
+1
Is there a useful comment on the @SuppressWarnings like in other files:
XSDHandler.java: 3028 DTMDocumentImpl: 1700 DOM2DTM.java: 1654 FilterExprWalker.java: 65
For the methods with long or very long switch statements, I added a note following the SuppressWarnings annotation to indicate where fallthrough would happen and in which case warnings were suppressed. But for the pretty short ones like the above, I thought it's quite obvious where fallthrough might happen, I didn't therefore add any comment.
A few of the added breaks would have been hiding bugs. It might be worth mentioning them in the issue.
I added a note to the issue.
Thanks, Joe
Roger
On 10/30/2017 1:09 PM, Lance Andersen wrote:
Hi Joe
The changes look OK
Best Lance
On Oct 30, 2017, at 1:03 PM, Joe Wang <huizhe.wang@oracle.com> wrote:
Hi,
Please review a cleanup of fallthrough and static warnings. For fallthrough, the majority of the changes are suppressing the warnings, while for static, replacing the instances with the class.
All jaxp tests and JCK passed.
JBS: https://bugs.openjdk.java.net/browse/JDK-8181155 webrevs: http://cr.openjdk.java.net/~joehw/jdk10/8181155/webrev/index.html
Thanks, Joe <http://oracle.com/us/design/oracle-email-sig-198324.gif> <http://oracle.com/us/design/oracle-email-sig-198324.gif> <http://oracle.com/us/design/oracle-email-sig-198324.gif> <http://oracle.com/us/design/oracle-email-sig-198324.gif>Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037 Oracle Java Engineering 1 Network Drive Burlington, MA 01803 Lance.Andersen@oracle.com <mailto:Lance.Andersen@oracle.com>
That's fine. Thanks On 10/30/2017 3:17 PM, Joe Wang wrote:
On 10/30/17, 11:14 AM, Roger Riggs wrote:
Hi Joe,
+1
Is there a useful comment on the @SuppressWarnings like in other files:
XSDHandler.java: 3028 DTMDocumentImpl: 1700 DOM2DTM.java: 1654 FilterExprWalker.java: 65
For the methods with long or very long switch statements, I added a note following the SuppressWarnings annotation to indicate where fallthrough would happen and in which case warnings were suppressed. But for the pretty short ones like the above, I thought it's quite obvious where fallthrough might happen, I didn't therefore add any comment.
A few of the added breaks would have been hiding bugs. It might be worth mentioning them in the issue.
I added a note to the issue.
Thanks, Joe
participants (4)
-
joe darcy
-
Joe Wang
-
Lance Andersen
-
Roger Riggs