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
