Code review for jigsaw/jake -> jdk9/dev sync up

[Alan Bateman]

Thanks for going through the changes, a few comment/replies below.


On 28/11/2016 22:22, Paul Sandoz wrote:
> :
>
>
> What happens if you pass a primitive array?
>
> I think you need to specify what happens if an array class is passed and how the target class is obtained, and an IAE if the "element type" of the array is a primitive class.
Yeah, it's something that I've been looking at too, mostly trying to 
decide whether to reject all array types for now. It is possible to 
specify an array class to Lookup::in but there are issues, it triggers 
at least one one assert that needs attention.

>
> (Separately i am looking forward to using this method to replace some Unsafe usages between Thread and other classes within the base module.)
>
>
> ModuleDescriptor
> —
>
>   241         private <T> Stream<String> toStringStream(Set<T> s) {
>   242             return s.stream().map(e -> e.toString().toLowerCase());
>   243         }
>   244
>   245         private <M> String toString(Set<M> mods, String what) {
>   246             return (Stream.concat(toStringStream(mods), Stream.of(what)))
>   247                     .collect(Collectors.joining(" "));
>   248         }
>
> The above occurs three times. Suggest moving to the enclosing class as package private static methods.
Thanks, this is clean-up that wasn't done after going through several 
iterations.



> :                    }
>
> Replace with
>
>    ResolvedModule rm = findInParent(dm);
>    if (rm != null)
>      other = rm.reference();
>
>
>   532             Set<ResolvedModule> requiresTransitive= new HashSet<>();
>
> Formating glitch for “=“

Thanks.

>
>
> ServiceLoader
> —
>
> 1331         @Override
> 1332         @SuppressWarnings("unchecked")
> 1333         public boolean tryAdvance(Consumer<? super Provider<T>> action) {
> 1334             if (ServiceLoader.this.reloadCount != expectedReloadCount)
> 1335                 throw new ConcurrentModificationException();
> 1336             Provider<T> next = null;
> 1337             if (index < loadedProviders.size()) {
> 1338                 next = (Provider<T>) loadedProviders.get(index);
> 1339             } else if (iterator.hasNext()) {
> 1340                 next = iterator.next();
> 1341             } else {
> 1342                 loadedAllProviders = true;
> 1343             }
> 1344             index++;
>
> Move the index increment into the top if block, thereby it only gets increment appropriately (no crazy overflow possible).
I agree, this could cause a problem in some extreme scenario.

>
>
> 1353         @Override
> 1354         public int characteristics() {
> 1355             // need to decide on DISTINCT
> 1356             // not IMMUTABLE as structural interference possible
> 1357             return Spliterator.ORDERED + Spliterator.NONNULL;
> 1358         }
>
> Should probably be consistent (subset of) with the characteristics reported for loadedProviders.stream(), thus DISTINCT would not be appropriate in this case unless you changed the optimal case of when all providers are loaded.
When all loaded then the characteristics come from ArrayList's 
spliterator and so will be ORDERED|SIZED|SUBSIZED. For consistency then 
we limit the above to ORDERED. I've been tempted to do more here but I 
think we need to get some real world usage of this method to see if 
anything is needed (esp. when the #providers is usually small).


> :
>
>
> 1552     public Optional<S> findFirst() {
> 1553         Iterator<S> iterator = iterator();
> 1554         if (iterator.hasNext()) {
> 1555             return Optional.of(iterator.next());
> 1556         } else {
> 1557             return Optional.empty();
> 1558         }
> 1559     }
>
>   return stream().findFirst() ?
>
The stream elements are Provider<S> rather than S so it could be 
stream().findFirst().map(Provider::get). The reason it was based on 
iterator is because it pre-dates stream but looking at it now then maybe 
findFirst() should go away as it's trivial to do with the new stream method.

-Alan