[foreign] RFR duplicates in named layouts have to be reported as error

[Maurizio Cimadamore]

Improved webrev:

http://cr.openjdk.java.net/~mcimadamore/panama/resolution_context_v3/

* Moved static resolution methods in Util to instance methods in 
LayoutResolver
* removed unused methods in LayoutResolver
* renamed resolution methods to just 'resolve'
* update copyright year

Maurizio


On 11/06/18 16:11, Sundararajan Athijegannathan wrote:
> Looks good.
>
> Minor: the new test has copyright year -> 2016
>
> -Sundar
>
> On 11/06/18, 8:09 PM, Maurizio Cimadamore wrote:
>> To be clear, here's what I'm proposing as a way to limit complexity:
>>
>> http://cr.openjdk.java.net/~mcimadamore/panama/resolution_context_v2/
>>
>> I've added a bunch of 'resolveLayout/Function' methods in Util (but 
>> they could also go in LayoutResolver...) - the binder calls them when 
>> needed. ABI is now free of unresolved thingies.
>>
>> Maurizio
>>
>>
>> On 11/06/18 14:31, Maurizio Cimadamore wrote:
>>> In hindsight, I believe the very first changeset submitted in this 
>>> thread was the correct one. Avoiding changes in the binder internal 
>>> by pushing more complexity onto the parser/unresolved layout API was 
>>> a siren song, which ultimately leads to a bad complexity ratio for 
>>> clients of the layout API.
>>>
>>> If the binder impl is more complex by having to deal with unresolved 
>>> layouts explicitly, well, that's not optimal (because it's a source 
>>> of potential bugs), but it's still way better than having _all_ 
>>> external clients having to care about resolution context.
>>>
>>> Perhaps, one improvement we could concretely make in this area is to 
>>> have an Util::resolve(Layout) which takes a partial layout and gives 
>>> you an instantiated layout. That way, we could force resolution in 
>>> places where we need to - not just on single unresolved nodes, but 
>>> on entire layout trees.
>>>
>>> I think this could help improving code uniformity and also reducing 
>>> impact of the changes - e.g. with this I think we could avoid 
>>> sending unresolved layouts into the ABI in the first place?
>>>
>>> Maurizio
>>>
>>>
>>> On 05/06/18 09:55, Sundararajan Athijegannathan wrote:
>>>> As I understand the main change (from what I had earlier) is to use 
>>>> the most enclosing class as context for layout resolution - 
>>>> regardless of whether it has NativeHeader annotation or not. 
>>>> Perhaps the tests can avoid NativeHeader annotation now.
>>>>
>>>> For eg. these
>>>>
>>>>  test/jdk/java/nicl/abi/sysv/x64/CallingSequenceBuilderTest.java
>>>>  test/jdk/java/nicl/types/StructTest.java
>>>>
>>>>  do not need NativeHeader annotation anymore.
>>>>
>>>> Rest is fine.
>>>>
>>>> -Sundar
>>>>
>>>> On 05/06/18, 2:14 PM, Maurizio Cimadamore wrote:
>>>>> Sorry - this is it:
>>>>>
>>>>> http://cr.openjdk.java.net/~mcimadamore/panama/resolution_context/
>>>>>
>>>>> Maurizio
>>>>>
>>>>>
>>>>> On 04/06/18 03:24, Sundararajan Athijegannathan wrote:
>>>>>>
>>>>>> Sorry I was on vacation.  The link you provided seems to be the 
>>>>>> initial named-layout implementation webrev. Will you please 
>>>>>> provide the correct link?
>>>>>>
>>>>>> -Sundar
>>>>>>
>>>>>> On 28/05/18, 6:04 PM, Maurizio Cimadamore wrote:
>>>>>>> Sorry, wrong webrev link, here's the correct one:
>>>>>>>
>>>>>>> http://cr.openjdk.java.net/~mcimadamore/panama/layout_resolver/
>>>>>>>
>>>>>>> Maurizio
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On 28/05/18 13:32, Maurizio Cimadamore wrote:
>>>>>>>> Here's a stab at the current approach; I started with your 
>>>>>>>> patch and tweaked in places.
>>>>>>>>
>>>>>>>> http://cr.openjdk.java.net/~mcimadamore/panama/panama-binder-v3.html 
>>>>>>>>
>>>>>>>>
>>>>>>>> I think the result is good; the problems you mention are 
>>>>>>>> resolved, and all use cases are supported.
>>>>>>>>
>>>>>>>> Let me know what you think.
>>>>>>>>
>>>>>>>> Maurizio
>>>>>>>>
>>>>>>>>
>>>>>>>> On 28/05/18 11:02, Maurizio Cimadamore wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 28/05/18 09:37, Maurizio Cimadamore wrote:
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On 28/05/18 07:39, Sundararajan Athijegannathan wrote:
>>>>>>>>>>> Isolated Structs will work if there are no name definitions 
>>>>>>>>>>> or references in its layout. If there are Unresolved name 
>>>>>>>>>>> references in its layout, then there has to be a 'context' 
>>>>>>>>>>> in which the names are defined/resolved. The context cannot 
>>>>>>>>>>> be entire Java process as it currently is! Two issues with 
>>>>>>>>>>> the current code: 
>>>>>>>>>> I agree with all you say. The only thing I'm disagreeing with 
>>>>>>>>>> is the fact that the context has to be the @NativeHeader 
>>>>>>>>>> annotation, as that suggests an header-centric nature that 
>>>>>>>>>> was never intended to be part of the design.
>>>>>>>>>>
>>>>>>>>>> Other, more explicit ways to define the context include an 
>>>>>>>>>> explicit annotation which list all the classes where 
>>>>>>>>>> dependent layout can be found. I think I'd like something 
>>>>>>>>>> like this better. Example (names TBD):
>>>>>>>>>>
>>>>>>>>>> @NativeStruct("[$(b)](a)")
>>>>>>>>>> @Friends(B.class)
>>>>>>>>>> interface A {
>>>>>>>>>>     B b();
>>>>>>>>>> }
>>>>>>>>>>
>>>>>>>>>> @NativeStruct("[$(a)](b)")
>>>>>>>>>> @Friends(A.class)
>>>>>>>>>> interface B {
>>>>>>>>>>     A a();
>>>>>>>>>> }
>>>>>>>>> Now that I have expressed this more explicitly, another idea 
>>>>>>>>> emerged - what if we assumed that, if no annotation is 
>>>>>>>>> provided, the context is the outermost class? That will work 
>>>>>>>>> neutrally with both struct and header interfaces...
>>>>>>>>>
>>>>>>>>> Maurizio
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Maurizio
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>
>>>
>>