On Mon, Nov 6, 2017 at 1:36 PM, David Holmes <david.holmes@oracle.com> wrote:

src/java.base/share/classes/java/util/concurrent/ConcurrentL inkedDeque.java

final Node<E> pred(Node<E> p) { ! Node<E> q = p.prev; ! return (p == q) ? last() : q; }

/** * Returns the first node, the unique node p for which: * p.prev == null && p.next != p --- 693,705 ---- * Returns the predecessor of p, or the last node if p.prev has been * linked to self, which will only be true if traversing with a * stale pointer that is now off the list. */ final Node<E> pred(Node<E> p) { ! if (p == (p = p.prev)) ! p = last(); ! return p; }

The original version is quite clear, the new version is trying to be far too clever with order of evaluation and to me is far less clear.

Well, this idiom is already in widespread use in these files, notably in succ, which was non-symmetrical with pred for no reason. The trickier code saves 2 bytes of bytecode; admittedly unlikely to make a difference; we are not near the 35-bytecode limit. The more common form of the idiom is if (p == (p = p.next)) continue restart; After 10 years working on lock-free queues it starts to look natural (but don't try it in C++!)

On 7/11/2017 8:17 AM, Martin Buchholz wrote:

Thanks for the review!

On Mon, Nov 6, 2017 at 1:36 PM, David Holmes <david.holmes@oracle.com <mailto:david.holmes@oracle.com>> wrote:

8189764: Miscellaneous changes imported from jsr166 CVS 2017-11

All seem okay. Though I'm curious about the changes from "catch(Throwable" to "catch(Exception" ?

There's a half-hearted attempt to appease http://errorprone.info/bugpattern/TryFailThrowable No actual bugs were fixed.

Hmmm. Not sure I see a problem if threadUnexpectedException just wraps and rethrows the original exception. On the other hand Errors are no longer being handled this way. Probably doesn't make a difference either way. Thanks, David

On 8 Nov 2017, at 14:20, Martin Buchholz <martinrb@google.com> wrote:

On Wed, Nov 8, 2017 at 2:09 PM, Paul Sandoz <paul.sandoz@oracle.com> wrote:

685 final Node<E> succ(Node<E> p) { 686 // TODO: should we skip deleted nodes here?

Is the comment still relevant?

It's still an open question, like what's the best GC strategy, or should ArrayList automatically shrink (as well as grow) its backing array?

Ok, i thought it might be somewhat related to skipping over null nodes and thus related to the linearizability fixes. Paul.