[foreign-memaccess] RFR 8226421: Remove LayoutPath API

[Jorn Vernee]

On 2019-06-25 19:18, Maurizio Cimadamore wrote:
>>> The only advantage I see is that it's not possible to write an
>>> ill-formed path (e.g. `foo[bar]`) with the API-based approach because
>>> the API is itself the grammar. Of course you trade this advantage off
>>> with compactness.
>> 
>> Yes, I was thinking of ill-formed paths.
> Btw, an API approach is not 100% free from this issue either - in the
> sense that you can still do something like
> 
> groupElement("foo").sequenceElement()
> 
> And then detect that the layout associated with 'foo' is not that of a
> sequence layout.
> 
> So, I believe the only real advantage in the API approach is that it
> avoids error due to mis-spelling of a path expression (e.g. syntax
> errors) - but the semantics errors that can arise are the same I
> think.

Yes, sorry, I'm talking about syntax errors. It's a minor problem.

>> 
>>>> 
>> Sounds good! But how about doing CompoundLayout.Path? Since paths only 
>> really seem to be useful for CompoundLayouts (and CompoundLayout is 
>> pretty small any ways).
> 
> That's what I meant - I also put together a webrev here:
> 
> http://cr.openjdk.java.net/~mcimadamore/panama/8226421_v2/
> 
> Overall, I like it. It seems a tidy approach, and ones that scale
> well. I think there's also a way to share paths, but you need to think
> in terms of higher-order functions - e.g.
> 
> UnaryOperator<Path> pathToStruct = ...
> UnaryOperator<Path> pathToStructDotA = path ->
> pathToStruct.apply(path).groupElement("a");
> UnaryOperator<Path> pathToStructDotB = path ->
> pathToStruct.apply(path).groupElement("b");
> 
> And, this doesn't preclude the string-y path, if we really felt that
> usability was a concern - e.g. we could have a parser that turns a
> path expression string into an UnaryOperator<Path> and then take it
> from there.

I like this approach. I think the move to CompoundLayout.Path nicely 
shows where paths are useful. Compared to the string-y approach, I think 
it's a bit more robust.

About the patch:

In LayoutPathImpl.java
   - copyright was removed. I don't think that was intentional?
   - the 'enclosing' field is unused, so I guess that can be removed.
   - in sequenceElement(long), I think the check should be `index >= 
seq.elementsSize().getAsLong()`, i.e. using greater than.

Rest looks good.

Jorn

> Maurizio
> 
> 
>> 
>> Jorn
>> 
>>> Thoughts?
>>> 
>>> Maurizio
>>> 
>>> 
>>>> 
>>>> Jorn
>>>> 
>>>> On 2019-06-20 00:34, Maurizio Cimadamore wrote:
>>>>> Hi,
>>>>> now that we have combinators to create memory access handles, it 
>>>>> seems
>>>>> like there is less need to have a very rich API to describe layout
>>>>> paths. But, we still want some higher level way to create var 
>>>>> handles
>>>>> (or compute offsets) from a complex layout expression...
>>>>> 
>>>>> Chatting with Brian and John, an idea emerged - to use a simple
>>>>> grammar to describe a layout path.
>>>>> 
>>>>> For instance, if you have the usual array of struct we used in 
>>>>> other examples:
>>>>> 
>>>>> [ 5 : [ x32 i32(value) ] ]
>>>>> 
>>>>> A path from the outermost sequence to the 'value' layout element 
>>>>> can
>>>>> be described with the following string:
>>>>> 
>>>>> "[].value"
>>>>> 
>>>>> So, if we had a language to describe layout paths, we can replace 
>>>>> the
>>>>> LayoutPath API with some methods which take a path expression as
>>>>> argument.
>>>>> 
>>>>> This is the patch:
>>>>> 
>>>>> http://cr.openjdk.java.net/~mcimadamore/panama/8226421/
>>>>> 
>>>>> I think the results are quite pleasing, and more compact than 
>>>>> before.
>>>>> I also placed the VarHandle accessors where they belong - that is, 
>>>>> an
>>>>> accessor taking a path expression in CompoundLayout. And then one 
>>>>> that
>>>>> just takes a carrier in ValueLayout (as no selection is possible 
>>>>> from
>>>>> a leaf).
>>>>> 
>>>>> I think this takes care of the last 'shaky' abstraction in the 
>>>>> memory
>>>>> access API.
>>>>> 
>>>>> Impl-wise, the expression parser builds on some of the work we've 
>>>>> done
>>>>> in the foreign branch - I've added a test which checks several 
>>>>> 'bad'
>>>>> things that can happen during parsing.
>>>>> 
>>>>> I've also added a javadoc section on 'path expressions' - which 
>>>>> shows
>>>>> the full grammar production for the path expression language.
>>>>> 
>>>>> One thing that I removed in the process was the ability to refer to
>>>>> struct components by index - in part because it felt a bit 
>>>>> unjustified
>>>>> (for now, at least), in part because it also felt a bit shaky 
>>>>> (w.r.t.
>>>>> unions).
>>>>> 
>>>>> Cheers
>>>>> Maurizio