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
