[modules-dev] Updated review request for 6628143, Service Loader support

[Mandy Chung]

Hi Dave,

I reviewed the implementation but not the tests.  Looks good in 
general.  My comments:

ServiceLoader.java
   line 261-262: these lines are not needed.

   line 204: since the service providers are loaded lazily, your 
previous spec change should be removed.

   line 315-340: would it be better to refactor the implementation of 
these constructors to avoid the duplication of the instantiation work?  
For example,
   private ServiceLoader(Class<S> svc, Repository repo) {
        this(svc, repo, null, service.getClassLoader().getModule(), false);
   }

and line 333 constructor will take an additional isCompound parameter.

  line 698: can it be replaced with mpClass.getSuperClass()?  Do you 
need to instantiate the object (i.e. can line 691 be removed?)

  line 1123: I think Repository.getBootstrapRepository() should be 
called here instead of Repository.getSystemRepository(), right?

ServiceProcessor.java
   line 95: can we just use the default implementation of this method by 
specifying the annotation like this?
  @SupportedAnnotationTypes({"java.util.Service", 
"java.util.ServiceProvider"})

If you override the method, should it return a unmodifiable Set?

  line 238: is calling toString() the correct way to get the class name? 
I think you may want to call e.getQualifiedName() instead.  Is it better 
to use Name instead of String as the key for the providedServices map 
instead?

  line 295: should r.close() be inside a finally clause so that it will 
still be closed when IOException is thrown.

RepositoryConfig.java
   line 283-286:  Why does configRepositories require 
"configureRepositories" ModuleSystemPermission?  In addition, this 
"configureRepositories" permission is not documented in the 
ModuleSystemPermission spec.

test/java/module/repository/LocalRepositoryTest.java
   The diff is empty.  I guess this file is not part of this review request.

Thanks
Mandy