On 09/01/2020 17:48, Chris Hegarty wrote:
Maurizio,
On 9 Jan 2020, at 14:36, Maurizio Cimadamore <maurizio.cimadam...@oracle.com>
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 )?
Yes, this looks like a mistake
MemoryLayout::offset - I find the throws clause hard to parse, maybe:
UnsupportedOperationException - if a layout with an unknown size is
encountered during traversal
I'll see what I can do here
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 ]
No, an out of bound address is allowed (and in fact sometimes is useful,
e.g. when an address is used as a 'limit' in a loop). So we don't ban
those on construction. Of course you dereference you get an exception.
SequenceLayout::withElementCount should probably specify IAE if given a
negative elementCount, right?
Whoops, yes
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?
Uhm - I would argue that the problem here lies in the class-level doc.
If a layout embeds a sequence w/o a size, then the layout does not have
a size (it might have a 'minimum size'). I'm more for fixing the
class-level assertion which seems too far reaching.
Maurizio
-Chris.
