Thanks for the feedback.
> On Mar 11, 2016, at 8:50 AM, Paul Sandoz <paul.san...@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
