Maurizio,
> On 9 Jan 2020, at 14:36, Maurizio Cimadamore <[email protected]>
> wrote:
>
> Hi,
> following the JEP 370 code review and CSR, certain comments that have not
> been addressed have been added to the tracker issue:
>
> https://bugs.openjdk.java.net/browse/JDK-8235837
>
> For further evaluation. After some discussion in the Panama channel (see [1]
> and [2]), the Panama team has come to the following conclusions:
>
> * the MemoryAddress::offset(long) method should be renamed to
> MemoryAddress::addOffset(long)
> * the MemorySegment::isAccessible() predicate should be dropped and replaced
> with the MemorySegment::ownerThread() accessor
> * a new predicate should be added to MemoryLayout, namely
> MemoryLayout::hasSize() to allow clients to establish as to whether a layout
> contains some unsized parts
> * a new method should be added to SequenceLayout, namely
> SequenceLayout::withElementCount, to create a new sequence layout with new
> element count
> * 2 new helper methods should be added to MemoryLayout, namely:
> - MemoryLayout::map - to allow transformation of a layout into a new
> layout (by replacing the sub-layout at given path with a new layout)
> - MemoryLayout::select - to allow selection of a sub-layout given a layout
> path
>
> Overall, we think this set of change strikes a good compromise between
> usability, readability and minimality.
>
> The javadoc for the layout path-accepting methods (both old and new ones) has
> been significantly refactored and improved - and the toplevel javadoc section
> on layout paths has been enhanced to cover all the concepts. The
> implementation for these methods has also been consolidated greatly, which
> revealed subtle bugs in the old implementation which have now been rectified
> (new tests have been added).
>
> Webrev is available here:
>
> http://cr.openjdk.java.net/~mcimadamore/8235837/
>
> Specdiff for changes here:
>
> http://cr.openjdk.java.net/~mcimadamore/8235837_specdiff/overview-summary.html
This mostly looks good. Some comments on the specification:
Is the example in Layout paths right?
long valueOffset = seq.offset(PathElement.sequenceElement(),
PathElement.groupElement("value"));
Doesn’t the sequence element need to have a specified position ( rather than
an unspecified element )?
MemoryLayout::offset - I find the throws clause hard to parse, maybe:
UnsupportedOperationException - if a layout with an unknown size is
encountered during traversal
MemoryAddress::addOffset ( I like the name ) allows a negative value. I get the
use case, so you can do:
ms.baseAddress().offset(10).offset(-5) is equivalent to
ms.baseAddress().offset(5)
, but what if one does ms.baseAddress().offset(-11) ?
Does addOffset need to throw an IAE? If so, then maybe it should be specified,
otherwise what is the purpose of allowing the offset to go negative ( or even
beyond the segments size? ) [ maybe we don’t care, since the bounds are checked
during access operations ]
SequenceLayout::withElementCount should probably specify IAE if given a
negative elementCount, right?
hasSize() - Suggest: "TELLS whether or not this layout has a KNOWN size".??
The reason I suggest “known” is that the class-level docs says that “all
layouts have a size”, just that the size is not known for an unbound sequence
layout. I would suggest a method name of “knownSize” or "sizeKnown", but that
maybe a little too far?
-Chris.
