Initial webrev with changes for JDK 9

[Paul Sandoz]

> On 11 Mar 2016, at 21:53, Mandy Chung <mandy.chung at oracle.com> wrote:
> 
> Thanks for the feedback.
> 
>> On Mar 11, 2016, at 8:50 AM, Paul Sandoz <paul.sandoz at oracle.com> wrote:
>> :
>> 
>> sun/util/locale/provider/ResourceBundleProviderSupport.java
>> —
>> 
>> 
>> 72                 // java.base may not be able to read the bundleClass's module.
>> 73                 PrivilegedAction<Void> pa1 = () -> { ctor.setAccessible(true); return null; };
>> 74                 AccessController.doPrivileged(pa1);
>> 75                 try {
>> 76                     return ctor.newInstance((Object[]) null);
>> 77                 } catch (InvocationTargetException e) {
>> 78                     Unsafe.getUnsafe().throwException(e.getTargetException());
>> 79                 } catch (InstantiationException | IllegalAccessException e) {
>> 80                     throw new InternalError(e);
>> 81                 }
>> 82             } catch (NoSuchMethodException e) {
>> 83             }
>> 84         }
>> 85         return null;
>> 86     }
>> 
>> 
>> Please avoid the use of Unsafe here, use a sneaky throw instead if necessary, or wrap.
> 
> I agree. It's also used from ResourceBundle.Control::newBundle. Since core reflection now gets readability for free, setAccessible is no longer needed [1]. So it can call Class::newInstance instead.  I fixed both places not to use Unsafe.
> 

Thanks!


>> 
>> j.l.ClassLoader
>> —
>> 
>> 1777         // define Package object if the named package is not yet defined
>> 1778         NamedPackage p = packages.computeIfAbsent(name,
>> 1779                                                   k -> NamedPackage.toPackage(name, m));
>> 1780         if (p instanceof Package)
>> 1781             return (Package)p;
>> 1782
>> 1783         // otherwise, replace the NamedPackage object with Package object
>> 1784         return (Package)packages.compute(name, this::toPackage);
>> 
>> You could merge the computeIfAbsent and compute into a single compute call if you so wish.
>> 
>> return (Package)packages.compute((n, p) -> {
>> // define Package object if the named package is not yet defined
>> if (p == null) return NamedPackage.toPackage(name, m);
>> // otherwise, replace the NamedPackage object with Package object
>> return (p instanceof Package) ? p : toPackage(n, p);
>> });
>> 
> 
> Thanks for the example.  I will keep it as is.

Ok. One advantage of the merged-approach is it’s atomic. The former may update an entry twice if there is a race, but that may not matter.


> 
>> 
>> j.l.r.Proxy
>> —
>> 
>> 636         private static final WeakCache<Module, List<Class<?>>, Class<?>> proxyCache =
>> 637             new WeakCache<>(new KeyFactory<Module>(),
>> 638                 new BiFunction<Module, List<Class<?>>, Class<?>>()  {
>> 639                     @Override
>> 640                     public Class<?> apply(Module m, List<Class<?>> interfaces) {
>> 641                         Objects.requireNonNull(m);
>> 642                         return defineProxyClass(m, interfaces);
>> 643                     }
>> 644             });
>> 
>> Wild idea: this feels like an equivalent of j.l.i.ClassValue for Module.
>> 
> 
> Feels like that.
> 
> This is the case when the proxy class is generated in a dynamic module case [1]. It’s a computed value  with the given list of proxy interfaces and the given defining class loader.
> 
> Each dynamic proxy is defined by a given class loader. The target Module where the proxy class may be a newly defined dynamic module per the given class loader.  So a given list of proxy interfaces + the defining class loader will map to one Module.
> 

Ok, so it takes some additional arguments to compute the value, so does not map quite so cleanly.



>> 
>> sun.invoke.util.VerifyAccess
>> —
>> 
>> 198                 while (c.isArray()) {
>> 199                     c = c.getComponentType();
>> 200                 }
>> 
>> This pattern is occurring a number of times.
>> 
>> Memo: add a method on Arrays, or somewhere…,
>> 
> 
> +1.  Perhaps Class::getBaseElementType.
> 

Yes, something like that.

Thanks,
Paul.