Thanks Lance!
Joe
On 6/21/19, 1:59 PM, Lance Andersen wrote:
The revised webrev looks fine Joe
On Jun 21, 2019, at 4:20 PM, Joe Wang <huizhe.w...@oracle.com
<mailto: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
<http://cr.openjdk.java.net/%7Ejoehw/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
<http://cr.openjdk.java.net/%7Ejoehw/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
<http://cr.openjdk.java.net/%7Ejoehw/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/
<http://cr.openjdk.java.net/%7Ejoehw/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>
