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

[Sundararajan Athijegannathan]

Incremental changes look good.

-Sundar

On 11/06/18, 10:36 PM, Maurizio Cimadamore wrote:
> 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
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>
>>>>
>>>
>