Re: RFR (JDK13/java.xml) 8224157: BCEL: update to version 6.3.1

[Joe Wang]

On 6/24/19, 12:00 PM, Daniel Fuchs wrote:

On 24/06/2019 18:24, Joe Wang wrote:
Keeping the code in sync with the BCEL (not perfect) source is in our 
interest to produce smaller changeset in any future updates. Would 
you be okay with the current webrevs?


Yes. This was just a remark that maybe the inconsistency should
be reported upstream (in case they want to fix it sometime in the
future). I wasn't suggesting to fix it downstream, or to postpone
the patch until it's fixed. Sorry if this wasn't clear.


Ah, ok. Thanks! :-)

Best,
Joe



best regards,

-- daniel

Re: RFR (JDK13/java.xml) 8224157: BCEL: update to version 6.3.1

[Daniel Fuchs]

On 24/06/2019 18:24, Joe Wang wrote:
Keeping the code in sync with the BCEL (not perfect) source is in our 
interest to produce smaller changeset in any future updates. Would you 
be okay with the current webrevs?


Yes. This was just a remark that maybe the inconsistency should
be reported upstream (in case they want to fix it sometime in the
future). I wasn't suggesting to fix it downstream, or to postpone
the patch until it's fixed. Sorry if this wasn't clear.

best regards,

-- daniel

Re: RFR (JDK13/java.xml) 8224157: BCEL: update to version 6.3.1

[Joe Wang]

On 6/24/19, 2:23 AM, Daniel Fuchs wrote:

On 21/06/2019 21:20, Joe Wang wrote:

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/



Thanks Joe!

The updated files look good.
Thumbs up!


Thanks!



Minor comment - that you might want to propagate upstream:


The hashCode impl is indeed inconsistent with equals. Looking at the 
issue [1], by using Objects.equals, they were fixing a possible NPE. 
Either case (the original or the use of Objects.equals) does not affect 
java.xml's specific usage of BCEL. The original JAXP team used to be 
part of Apache projects, but I myself hasn't been. While we would like 
it to be perfect (in format, and in issues such as this), I think we 
have to live with it, acknowledging it but knowing the edge case does 
not affect our usage.


Keeping the code in sync with the BCEL (not perfect) source is in our 
interest to produce smaller changeset in any future updates. Would you 
be okay with the current webrevs?


[1] https://issues.apache.org/jira/browse/BCEL-297

Best regards,
Joe



 @Override
-public boolean equals(final Object o1, final Object o2) {
+public boolean equals( final Object o1, final Object o2 ) {
 final JavaClass THIS = (JavaClass) o1;
 final JavaClass THAT = (JavaClass) o2;
-return THIS.getClassName().equals(THAT.getClassName());
+return Objects.equals(THIS.getClassName(), 
THAT.getClassName());

 }

+
 @Override
-public int hashCode(final Object o) {
+public int hashCode( final Object o ) {
 final JavaClass THIS = (JavaClass) o;
 return THIS.getClassName().hashCode();
 }

I've seen that idiom at a few places: equals was modified to
use Objects.equals(), but hashCode was not.

1. equals will throw if it is passed a null THIS or THAT (is
   that intented?)
2. equals seems to think that getClassName() can return null,
   whereas hashCode obviously expect that it will be non null.

I cannot say whether that's right or wrong, but I can smell something...
Maybe having a look at the issue that triggered the use of
Objects::equals could shed some light on this. It doesn't seem worse
than what was there before - it's just a bit odd.


best regards,

-- daniel

Re: RFR (JDK13/java.xml) 8224157: BCEL: update to version 6.3.1

[Daniel Fuchs]

On 21/06/2019 21:20, Joe Wang wrote:

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/



Thanks Joe!

The updated files look good.
Thumbs up!

Minor comment - that you might want to propagate upstream:

 @Override
-public boolean equals(final Object o1, final Object o2) {
+public boolean equals( final Object o1, final Object o2 ) {
 final JavaClass THIS = (JavaClass) o1;
 final JavaClass THAT = (JavaClass) o2;
-return THIS.getClassName().equals(THAT.getClassName());
+return Objects.equals(THIS.getClassName(), 
THAT.getClassName());

 }

+
 @Override
-public int hashCode(final Object o) {
+public int hashCode( final Object o ) {
 final JavaClass THIS = (JavaClass) o;
 return THIS.getClassName().hashCode();
 }

I've seen that idiom at a few places: equals was modified to
use Objects.equals(), but hashCode was not.

1. equals will throw if it is passed a null THIS or THAT (is
   that intented?)
2. equals seems to think that getClassName() can return null,
   whereas hashCode obviously expect that it will be non null.

I cannot say whether that's right or wrong, but I can smell something...
Maybe having a look at the issue that triggered the use of
Objects::equals could shed some light on this. It doesn't seem worse
than what was there before - it's just a bit odd.


best regards,

-- daniel

Re: RFR (JDK13/java.xml) 8224157: BCEL: update to version 6.3.1

[Joe Wang]

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 > 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







Lance 
Andersen| Principal Member of Technical Staff | +1.781.442.2037

Oracle Java Engineering
1 Network Drive
Burlington, MA 01803
lance.ander...@oracle.com 

Re: RFR (JDK13/java.xml) 8224157: BCEL: update to version 6.3.1

[Lance Andersen]

The revised webrev looks fine Joe
> On Jun 21, 2019, at 4:20 PM, Joe Wang  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
>>> 
>> 

 
  

 Lance Andersen| 
Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering 
1 Network Drive 
Burlington, MA 01803
lance.ander...@oracle.com 

Re: RFR (JDK13/java.xml) 8224157: BCEL: update to version 6.3.1

[Lance Andersen]

Hi Joe,

Overall looks OK.   Its unfortunate to loose the formatting but given it is not 
upstream, best to not have the additional work of going through this exercise 
each time.

I have no additional changes to propose outside of what Daniel caught.

Even though there are a large number of files, the updates/impact would be 
minimal if approved for JDFK 13.  It might be easier though to just target JDK 
14 given where we are in the release cycle.

HTH

Lance
> On Jun 20, 2019, at 6:45 PM, 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
> 

 
  

 Lance Andersen| 
Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering 
1 Network Drive 
Burlington, MA 01803
lance.ander...@oracle.com 

Re: RFR (JDK13/java.xml) 8224157: BCEL: update to version 6.3.1

[Joe Wang]

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

Re: RFR (JDK13/java.xml) 8224157: BCEL: update to version 6.3.1

[Daniel Fuchs]

Hi Joe,

I promise, I will not grumble about the formatting and
weird white spaces [grumble grumble]...

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?

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.


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?

Otherwise I believe the rest looks mostly OK.

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

RFR (JDK13/java.xml) 8224157: BCEL: update to version 6.3.1

[Joe Wang]

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