Initial webrev with changes for JDK 9

[Mandy Chung]

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.

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

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

> 
> :
> Use Stream.flatMap e.g.
> 
>  interfaces.stream().flatMap(i -> Stream.of(i.getMethods()).
>    flatMap(m -> f(m) /*extract all types on method*/).
>    map(…).filter(…).collect(toSet())
> 
> Stream<Class<?>> f(Method m) {
>  return Stream.of(new Class[] {m.getReturnType()}, m.getParameterTypes(), m.getExceptionTypes()).
>    flatMap(a -> Stream.of(a));
> }

Updated.

> 
> Consider using computeIfAbsent. At the moment no need for AtomicInteger.
> 
> 

Updated.  I probably intended to convert that when converting to AtomicInteger.

> 1055     @CallerSensitive
> 1056     public static Object newProxyInstance(ClassLoader loader,
> 1057                                           Class<?>[] interfaces,
> 1058                                           InvocationHandler h) {
> 1059         Objects.requireNonNull(h);
> 1060
> 1061         final List<Class<?>> intfs = Arrays.asList(interfaces.clone());
> 
> Use List.of, which will also clone? This presumes that there are no null values in the array.

Updated.  Happy to use the new JDK 9 List.of method (the code predates List.of). Yes it will clone and do null check. 
> 
> 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.

> 
> sun.util.resources.LocaleData
> —
> 
>  77     private static final Map<String, List<Locale>> candidatesMap = new ConcurrentHashMap<>();
> 
> candidatesMap -> CANDIDATES_MAP
> 

Changed.

Alan has covered the rest.

Mandy

[1] http://mail.openjdk.java.net/pipermail/jpms-spec-experts/2016-March/000248.html
[2] http://download.java.net/java/jigsaw/docs/api/java/lang/reflect/Proxy.html#membership