RFR(jdk10/jaxp) 8163121: BCEL: update to the latest 6.0 release

Fri Aug 11 10:31:40 UTC 2017

Hi Joe,

Thanks, looks fine now!

best regards,

-- daniel

On 10/08/2017 04:13, huizhe wang wrote:
> 
> On 8/9/2017 6:30 AM, Daniel Fuchs wrote:
>> Hi Joe,
>>
>> This not an easy patch to review ;-)
> 
> Indeed, took several minutes just to copy the webrevs :-)
> 
>>
>> Thanks for explaining how you arrived at the final result.
>> The method you used as described below seems right.
>>
>> I haven't clicked through all the files, but instead I had a
>> look at  all the JIRA issues revealed by:
>>
>> hg log -k share/classes/com/sun/org/apache/bcel | grep summary | grep 
>> -v Merge
>>
>> to see if any of them might have been reverted/invalidated.
>> So far so good, most of them still seem to be present in
>> your changes, but I have a doubt about these two:
>>
>> https://bugs.openjdk.java.net/browse/JDK-8062608
>> https://bugs.openjdk.java.net/browse/JDK-8064516
>>
>> Has the issue been fixed upstream in a different way, or
>> is the issue no longer relevant, or is it going to appear
>> again with this patch?
> 
> Good catch!  The original patch came from BCEL, so I indeed assumed this 
> update would get things the right way. As I double-checked, it turns out 
> the additional patches as you listed above fixed issues that BCEL 6.0 
> hasn't. These issues were revealed by JRocket, did not affect JAXP, 
> however, they are still good changes that we shall keep. I've gone 
> through both of the above to put these changes back into the code.
> 
>>
>> Also looking at the JDK fixes brought my attention to
>> java.xml/share/classes/jdk/xml/internal/SecuritySupport.java
>>
>> Your changes are adding these two methods:
>> I don't know what they are for - and I am not sure they would
>> work (the brutal cast to ListResourceBundle is dubious).
>> Fortunately they don't seem to be called by the new code (I
>> grepped the global patch file). Can you remove them?
> 
> This is because I removed 
> com/sun/org/apache/bcel/internal/util/SecuritySupport.java that is no 
> longer used by any classes in our version of BCEL. However, a XPath 
> class (XPATHMessages.java) was still using it, I therefore moved the 
> getResourceBundle method that was used into the jdk util class before 
> removing the one in bcel.
> 
> The ListResourceBundle cast was needed since the XPATHMessages variable 
> was originally declared ListResourceBundle. I've changed that to 
> ResourceBundle and removed the cast.
> 
> Here's the updated webrev:
> http://cr.openjdk.java.net/~joehw/jdk10/8163121/webrev01/
> 
> Thanks,
> Joe
> 
>>
>>  234     /**
>>  235      * Gets a resource bundle using the specified base name, the 
>> default locale, and the caller's class loader.
>>  236      * @param bundle the base name of the resource bundle, a 
>> fully qualified class name
>>  237      * @return a resource bundle for the given base name and the 
>> default locale
>>  238      */
>>  239     public static ListResourceBundle getResourceBundle(String 
>> bundle) {
>>  240         return getResourceBundle(bundle, Locale.getDefault());
>>  241     }
>>  242
>>  243     /**
>>  244      * Gets a resource bundle using the specified base name and 
>> locale, and the caller's class loader.
>>  245      * @param bundle the base name of the resource bundle, a 
>> fully qualified class name
>>  246      * @param locale the locale for which a resource bundle is 
>> desired
>>  247      * @return a resource bundle for the given base name and locale
>>  248      */
>>  249     public static ListResourceBundle getResourceBundle(final 
>> String bundle, final Locale locale) {
>>  250         return 
>> AccessController.doPrivileged((PrivilegedAction<ListResourceBundle>) 
>> () -> {
>>  251             try {
>>  252                 return 
>> (ListResourceBundle)ResourceBundle.getBundle(bundle, locale);
>>  253             } catch (MissingResourceException e) {
>>  254                 try {
>>  255                     return 
>> (ListResourceBundle)ResourceBundle.getBundle(bundle, new Locale("en", 
>> "US"));
>>  256                 } catch (MissingResourceException e2) {
>>  257                     throw new MissingResourceException(
>>  258                             "Could not load any resource bundle 
>> by " + bundle, bundle, "");
>>  259                 }
>>  260             }
>>  261         });
>>  262     }
>>
>> best regards,
>>
>> -- daniel
>>
>> On 09/08/2017 00:43, huizhe wang wrote:
>>> Please review an update to BCEL release 6.0 [1].
>>>
>>> The sources are basically that from the release bundle with the 
>>> following exceptions:
>>>
>>>   * Classes that were not included in the original version are excluded;
>>>   * Classes that are deprecated in BCEL 6.0 are removed;
>>>   * Classes that are used only for the above classes are also removed;
>>>   * Many fields were deprecated in BCEL 6.0 in order to discourage
>>>     direct references, in the JDK, I've removed such deprecation and
>>>     instead made them private/final.
>>>   * BCEL 6.0 applied 'final' to global/local variables pretty
>>>     extensively, which were the only changes to many of the classes.
>>>   * A few Transform classes were updated to adopt to BCEL 6.0.
>>>   * JDK-8162527 [JAXP] XSLT: Investigate why bumping the default class
>>>     file version to 49 (Java 5.0) or higher for bytecode generation
>>>     doesn't work [2] has not been resolved as of this update. Additional
>>>     change [3] is needed before we can move up to possibly Java 5.0.
>>>
>>>
>>> BCEL has been a very old component. This update therefore removed a 
>>> large amount of warnings (900+), which is very helpful to the 
>>> cleaning effort.
>>>
>>> All tests passed, including the JCK.
>>>
>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8163121
>>> webrevs: http://cr.openjdk.java.net/~joehw/jdk10/8163121/webrev/
>>>
>>>
>>> [1] https://archive.apache.org/dist/commons/bcel/RELEASE-NOTES.txt
>>> [2] https://bugs.openjdk.java.net/browse/JDK-8162527
>>> [3] https://bugs.openjdk.java.net/browse/JDK-8185991
>>>
>>> Thanks,
>>> Joe
>>>
>>
> 