[foreign-abi] RFR: Revisit FunctionDescriptor

[Maurizio Cimadamore]

On 04/02/2020 12:15, Maurizio Cimadamore wrote:
> What about having two factory methods (as we have in other layout 
> classes) - e.g.
>
> FunctionDescriptor.of( ... )
> FunctionDescriptor.ofVariadic(...)

Nevermind - we also have overloads for void/non void.

Let's just remove variadic-ness info for now. As I said, jextract is 
fully able to recover that info from the jextract declaration API. 
Before, we needed that info _in the layouts/descriptors_ because they 
were the only thing that came out of jextract - and they were what drove 
the binder. Now the story is more nuanced and jextract will be able to 
generate the right MH w/o needed any special hook in function descriptor.

Maurizio

>
> Maurizio
>
> On 04/02/2020 10:14, Jorn Vernee wrote:
>> Ah, I missed the use in RuntimeHelper. That seems to be the only real 
>> use case.
>>
>> Wrt using annotations instead, I guess what I'm saying is; I don't 
>> see that much value in being able to attach this single bit of 
>> information to a FunctionDescriptor, but I see more value in being 
>> able to attach any kind of information to a FunctionDescriptor. Since 
>> the ABI machinery doesn't use it either, the variadic flag seems like 
>> an arbitrary appendage. Something like that feels more at home as an 
>> annotation (a mechanism for arbitrary appendages).
>>
>> Though, my main gripe is with having to read (and write) the extra 
>> boolean argument all the time when a FunctionDescriptor is created, 
>> even though this extra visual information is rarely relevant (and 
>> indeed, it is irrelevant if you're reading just foreign-abi code). 
>> That's the main thing I'd like to remove. Since the flag is currently 
>> being used, I'd suggest replacing the boolean flag passed to the 
>> factory with an asVariadic() method.
>>
>> New webrev: 
>> http://cr.openjdk.java.net/~jvernee/panama/webrevs/cleanup_fd/webrev.01/
>>
>> Thoughts?
>>
>> Jorn
>>
>> On 03/02/2020 19:10, Maurizio Cimadamore wrote:
>>> (without having looked at the code) - jextract runtime support is 
>>> currently using the varargs info to generate varargs MethodHandle 
>>> which do the specialization. As you say, we can tackle that problem 
>>> with annotations, although I'm not sure of what's the net gain in 
>>> moving things around. At this point I guess I'd prefer jextract to 
>>> not generate any magic annotation - the Declaration.Function class 
>>> knows whether a function is a varargs or not, so there's no need to 
>>> truck that info into the runtime.
>>>
>>> Maurizio
>>>
>>> On 03/02/2020 16:19, Jorn Vernee wrote:
>>>> Hi,
>>>>
>>>> Please review the following patch that revisits FunctionDescriptor: 
>>>> 1. Adds appendArgumentLayouts, and changeReturnLayout combinator 
>>>> methods. 2. Removes the variadic flag. This flag was not really 
>>>> being used in practice, since we can not link variadic functions 
>>>> directly any ways. Using the new appendArgumentLayouts method we 
>>>> can 'specialize' a base FD with different argument layouts 
>>>> representing vararg arguments. I think if a similar flag is needed 
>>>> in the future, we should consider adding annotations to 
>>>> FunctionDescriptor, and making it an annotation instead, but 
>>>> currently it doesn't seem like it's pulling it's weight by being in 
>>>> the API.
>>>>
>>>> Webrev: 
>>>> http://cr.openjdk.java.net/~jvernee/panama/webrevs/cleanup_fd/webrev.00/ 
>>>>
>>>> Bugs: https://bugs.openjdk.java.net/browse/JDK-8238226, 
>>>> https://bugs.openjdk.java.net/browse/JDK-8237580
>>>>
>>>> Again, this patch applies on top of the Binding cleanup patch.
>>>>
>>>> Thanks,
>>>> Jorn
>>>>