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

[Dave Bristor]

Hi Mandy,

Thanks for the careful review.

Mandy Chung wrote:
> 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.

Removed.

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

Could you be more specific?  The javadoc describes how services & providers 
are loaded from repositories, and this still holds.  The implementation is 
completely lazy :-) so the pre-existing comment about laziness still applies 
too.  I think the new text in the webrev was originally suggested by Stanley.

For now, I didn't make any changes.

>   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.

Fixed.

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

Instantiation not needed, you're right!  Fixed.

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

Yep, definitely: changed.

> 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?

I hadn't noticed the existence of that annotation!  Fixed, and so the method 
is removed.

>  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?

Fixed by using Name and getQualifiedName.  Look for a toString() of the Name 
instance in createServices().

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

Fixed.

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

I reconsidered this, and believe the check is not necessary and the method 
should not be public.  I noticed that javadoc, especially for 
configRepositories(Properties), needed updating, and fixed that also.

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

Sorry about that; I'll keep it out of future reviews.

> 
> Thanks
> Mandy
>