[foreign-memaccess] RFR 8236267: Cleanup support for layouts with undefined size

[Jorn Vernee]

Hi,

This looks good. Some nits:

MemoryLayout.java
- L91: "Layout paths are useful in order to e.g. to obtain offsets..." 
-> "Layout paths are for example useful to obtain offsets..."
- L165: "A layout does not a specified size..." -> "A layout does not 
have a specified size..."
- L168: "@return {@code true}, if this layout have a specified size." -> 
"@return {@code true}, if this layout has a specified size."
- L316: "lauout" -> "layout"
- General on this file:
   1.) Can you factor out the `case SEQUENCE_ELEMENT_INDEX, 
SEQUENCE_RANGE: throw new IllegalArgumentException(...);` check into a 
helper method, since it is duplicated several times? (maybe 
checkBounded(pathElement)).
   2.) Several places in the javadoc metion something like "... the 
layout path obtained by concatenating the path elements ...", which 
seems like a bit of a mouthful and triggers the question "what does it 
mean to 'concatenate' path elements?". I think just talking about a 
"layout path" would be good enough since the class javadoc already 
explains: "Layout paths are typically expressed as a sequence of one or 
more {@link PathElement} instances."

SequenceLayout.java
- L99: "Obntains a new sequence layout with same..." -> "Obtains a new 
sequence layout with the same..."

Cheers,
Jorn

On 19/12/2019 15:52, Maurizio Cimadamore wrote:
> Hi,
> I decided to go ahead and create a separate issue for dealing with 
> unbounded sequence layouts, since it seems like the API has some 
> aspects which makes it difficult to deal with them:
>
> * it is not possible to establish whether a layout has a size, and 
> hence whether e.g. passing it to MemorySegment::allocateNative will 
> blow up
> * there's no way, given a layout which contains one or more unbound 
> sequences to replace them with bounded ones - w/o recreating 
> everything from scratch
>
> This patch fixes these problems by:
>
> * adding an hasSize() predicate to all layouts
> * adding a way to 'map' a layout given a path - that is, create a new 
> layout where one nested element has been replaced using a given unary 
> operator
>
> Separately, I've also added a way to 'select' a nested layout given a 
> path (which is not strictly necessary, but seems a nice to have).
>
> Webrev:
>
> http://cr.openjdk.java.net/~mcimadamore/panama/8236267/
>
> I've also updated javadoc here:
>
> http://cr.openjdk.java.net/~mcimadamore/panama/memaccess_javadoc/jdk/incubator/foreign/package-summary.html 
>
>
> I've done a significant cleanup of the javadoc associated with the 
> methods accepting layout paths (as well as their implementations), as 
> there were a number of issues:
>
> * the javadoc were not documenting that some path element selection 
> could fail (e.g. if traversing a layout with no size)
> * related, the newly created methods should be robust to the lack of 
> size in a traversed layout (as they are more about selection than 
> about precise offset computation)
> * we were not clear (both in spec and impl) re. which PahElement are 
> allowed on each of the path-accepting methods. For instance, passing 
> PathElement.sequenceElement() to MemoryLayout::offset() makes no 
> sense, as it does not select a single layout element with given 
> offset. Similarly, the newly added methods have other restrictions 
> which are documented (and tested)
> * I've added more examples in the MemoryLayout toplevel javadoc, to 
> show how layout paths can be used
>
> Cheers
> Maurizio
>
>
>