On Thu, Nov 17, 2016 at 12:03 PM, Paul Sandoz <paul.san...@oracle.com>
wrote:
>
> Perhaps consolidate the repeated mini bit set methods as package private
> static methods on AbstractCollection? Unclear where the j.u.concurrent ones
> should go though...
>
Deferred.
> It’s purely a subjective thing but i would be inclined to change the
> variable name “deathRow" to something more neutral such as “removedBitSet”.
>
It's certainly evocative! Especially given that they may get a reprieve
from a following predicate!
> The 2 layer nested loop is quite clever, certainly twists the mind when
> first encountered. It’s arguably more readable if there are two separate,
> (not-nested) loops, but that also requires two explicit invocations of the
> action, which may perturb the performance characteristics.
>
After you've written a hundred of these loops, they start to look rather
natural/idiomatic. I think these nested loops should be the canonical way
to implement traversals of circular buffers in any programming language.
The inner loops seem optimal. It seems unlikely we've invented something
new here, but maybe...
> ArrayDeque
> —
>
> 188 public ArrayDeque(int numElements) {
> 189 elements = new Object[Math.max(1, numElements + 1)];
> 190 }
>
> What if Integer.MAX_VALUE is passed?
>
Now tries (and fails!) to allocate a maximal array.
> 202 public ArrayDeque(Collection<? extends E> c) {
> 203 elements = new Object[c.size() + 1];
> 204 addAll(c);
> 205 }
>
> What if c.size() returns Integer.MAX_VALUE?
>
>
Now delegates to the other constructor.
>
> 226 * Adds i and j, mod modulus.
> 227 * Precondition and postcondition: 0 <= i < modulus, 0 <= j <=
> modulus.
> 228 */
> 229 static final int add(int i, int j, int modulus) {
>
> Is that upper bound correct for j? 0 <= j < modulus
>
j renamed to distance, to emphasize asymmetry.
Although in this file, distance will never equal modulus.
>
> 317 c.forEach(e -> addLast(e));
>
> this::addLast, up to you which you prefer
>
>
Meh. Left as is; another vote could tip to the other side.
>
> 843 public boolean tryAdvance(Consumer<? super E> action) {
> 844 if (action == null)
> 845 throw new NullPointerException();
> 846 int t, i;
> 847 if ((t = fence) < 0) t = getFence();
>
> Is that for optimisation purposes, since the same check is also performed
> in getFence? If so that seems like overkill
>
>
done.
>
> 848 if (t == (i = cursor))
> 849 return false;
> 850 final Object[] es;
> 851 action.accept(nonNullElementAt(es = elements, i));
> 852 cursor = inc(i, es.length);
>
> Recommend updating cursor before the action
>
>
done.
>
> 853 return true;
> 854 }
>
>
>
> Collection8Test
> —
>
> 429 public void testRemoveAfterForEachRemaining() {
>
> Are tests run by default with testImplementationDetails == true? I am
> guessing so.
>
>
We run various combinations in jtreg mode.
* @run junit/othervm/timeout=1000 -Djsr166.testImplementationDetails=true
JSR166TestCase
* @run junit/othervm/timeout=1000
-Djava.util.concurrent.ForkJoinPool.common.parallelism=0
-Djsr166.testImplementationDetails=true JSR166TestCase
* @run junit/othervm/timeout=1000
-Djava.util.concurrent.ForkJoinPool.common.parallelism=1
-Djava.util.secureRandomSeed=true JSR166TestCase
The original target for these tests are the jck, and that is expected to
run with the default testImplementationDetails=false
