> On 11 Mar 2016, at 21:53, Mandy Chung <mandy.ch...@oracle.com> wrote:
>
> 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.
>
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.
