8173393: Module system implementation refresh (2/2017)

[Alan Bateman]

On 10/02/2017 10:00, Peter Levart wrote:

>
> First, just a nit...
>
> java.lang.module.Resolver:
>
>  320     private void addFoundModule(ModuleReference mref) {
>  321         ModuleDescriptor descriptor = mref.descriptor();
>  322         nameToReference.put(descriptor.name(), mref);
>  323
>  324         if (descriptor.osName().isPresent()
>  325                 || descriptor.osArch().isPresent()
>  326                 || descriptor.osVersion().isPresent())
>  327             checkTargetConstraints(descriptor);
>  328     }
>
> ...perhaps more correct would be to reverse the order: 1st check 
> target constraints and then add descriptor to map. Otherwise in case 
> of check failure, you end up with a Resolver instance that is poisoned 
> with incompatible module descriptor. Maybe you always throw away such 
> Resolver if this method fails, but maybe someone will sometime try to 
> recover and continue to use the Resolver for rest of modules?
Yes, fair point, it would be better to do this check first. I don't 
think we have any issue now because this is an internal class and the 
resolver is always thrown away after an exception. The ModuleTarget 
attribute isn't completely firmed up yet so I expect we will be back to 
this code soon.


>
>
> ...then something more involving...
>
> java.lang.reflect.AccessibleObject::canAccess could share access cache 
> with internal method checkAccess. In particular since one might use 
> this new method in scenarios like:
>
> Method method = ...;
>
> if (method.canAccess(target)) {
>     method.invoke(target, ...);
> } else {
or the other usage I would expect to see is:

   if (method.canAccess(target) || method.tryAccessible()) { .. }

> ...
>
> Here's what you could do:
>
> http://cr.openjdk.java.net/~plevart/jdk9-jake/AccessibleObject.canAccess_caching/webrev.01/ 
>
Good idea. Do you want to create an issue for this and follow-up on 
core-libs-dev? The changes are in jdk9/dev now so it can follow. A minor 
nit is that the InternalError was useful to catch bad changes, a NPE 
could work too but it should never happen.

-Alan