The revised webrev looks fine Joe > On Jun 21, 2019, at 4:20 PM, Joe Wang <huizhe.w...@oracle.com> wrote: > > > > On 6/21/19, 11:59 AM, Daniel Fuchs wrote: >> Hi Joe, >> >> I promise, I will not grumble about the formatting and >> weird white spaces [grumble grumble]... > > Someone at BCEL needs to re-format the source with a modern IDE ;-) > >> >> http://cr.openjdk.java.net/~joehw/jdk13/8224157/webrev/src/java.xml/share/classes/com/sun/org/apache/bcel/internal/classfile/ConstantUtf8.java.frames.html >> >> >> Is the new import really needed here? > > No, good catch. >> >> http://cr.openjdk.java.net/~joehw/jdk13/8224157/webrev/src/java.xml/share/classes/com/sun/org/apache/bcel/internal/classfile/JavaClass.java.frames.html >> >> >> 232 if (!dir.isDirectory()) { >> >> I see use of SecuritySupport was dropped here and I think I agree, >> as I don't see why isDirectory() would require it if mkdirs() doesn't. > > I honestly don't remember why I added that in the previous update. But you're > right, and all test run proved it. > >> >> >> http://cr.openjdk.java.net/~joehw/jdk13/8224157/webrev/src/java.xml/share/classes/com/sun/org/apache/bcel/internal/classfile/LineNumberTable.java.udiff.html >> >> >> Are the changes in toString() actually correct? > > No, it's not. Good catch again! The change in the last update was correct >> >> Otherwise I believe the rest looks mostly OK. > > Thanks Daniel! Here's an updated webrev: > > Full webrev (for the record) > http://cr.openjdk.java.net/~joehw/jdk13/8224157/webrev_02/ > > A short version of webrev_02 that includes the only files mentioned in this > review: > http://cr.openjdk.java.net/~joehw/jdk13/8224157/webrev_02_short/ > > > I'm re-running all tests. > > Best regards, > Joe > >> >> best regards, >> >> -- daniel >> >> On 20/06/2019 23:45, Joe Wang wrote: >>> Please review an update to BCEL 6.3.1. This changeset will go into JDK13 if >>> approved, 14 if not. >>> >>> 1. Format >>> The changeset looks big, the majority of the changes however were only >>> different in format (i.e. Const.java). Unlike previous updates, I'm leaving >>> the format as they are in the upstream source so that they won't show up in >>> future updates. The only change I made were those that had extremely long >>> lines. >>> >>> 2. Exclusions >>> Since the BCEL component is for xml transform only, several classes, >>> that were not in the JDK, were excluded. Among them, JavaWrapper.java for >>> example can be problematic as it may use an user-specified classloader to >>> load arbitrary classes. It and related classes were therefore excluded. >>> >>> 3. Warnings >>> Warnings were the main reason for the changes made to the original >>> source. It has been done in the previous update. For this update therefore, >>> I only had to re-apply them after making copies of the upstream source. >>> Still, I updated the LastModified field to indicate a modification to the >>> original source. >>> >>> 4. Deprecated fields to private >>> Deprecated fields in the original source were changed to private ones >>> in previous update. The changes are inherited in this update. Again, the >>> LastModified fields are also updated. >>> >>> 5. Test >>> Since the update does not affect the usage of the BCEL component, it is >>> essential to pass all of the existing tests. I've run the tests multiple >>> times and noted that all of the XML functional and unit tests passed, so >>> were JCK XML tests. I've also done a comparison between builds before and >>> after applying the BCEL update, and found no change in performance. >>> >>> JBS: https://bugs.openjdk.java.net/browse/JDK-8224157 >>> webrevs: http://cr.openjdk.java.net/~joehw/jdk13/8224157/webrev/ >>> >>> 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.ander...@oracle.com <mailto:lance.ander...@oracle.com>
