[foreign-abi] RFR: Rename SystemABI to ForeignLinker, and move C support to a separate class.

[Maurizio Cimadamore]

On Mon, 18 May 2020 10:55:32 GMT, Maurizio Cimadamore <mcimadamore at openjdk.org> wrote:

>> Hi,
>> 
>> This patch renames SystemABI to ForeignLinker, and moves the C support, namely layout constants and the getSystemABI
>> factory, to a new class named `C`.
>> This is an effort to untangle the otherwise ABI/Language agnostic API of SystemABI from APIs that serve C specifically.
>> 
>> The rename from SystemABI to ForeignLinker attempts to make it clear that there is not a single ABI per system, but
>> there can be multiple. Although the same holds for C, in practice there is one de facto ABI per system, so we still
>> keep the getSystemLinker (renamed from getSystemABI) factory for C.  The new name also better reflects what the class
>> does; it links a native function as a MethodHandle, or links a Java function as a native function pointer. The overall
>> theme being linking.  I've also removed some of the ABI name constants that were in SystemABI previously, as they were
>> unused.
>> (FYI, the name change in the diff on GitHub from SystemABI -> C seems to have been inferred automatically, and is
>> incorrect. The actual rename is from SystemABI -> ForeignLinker, and the C class was added separately).
>> Thanks,
>> Jorn
>
> Looks good - there are few things that need more thinking:
> * the name `C` seems thin. It doesn't describe very well what the class is for. Maybe `CSupport` or something like that
>   might be more expressive
> * if we have already C somewhere in the class name, there's a question as to whether constants should drop their C-ness
>   (e.g. `C.C_BOOL` looks odd).
> * I wonder if, now that we have a dedicated C class, helpers functions to read/write strings shouldn't just go in there
>   (e.g. take Cstring and expand its static helpers onto the new class).

> if we have already C somewhere in the class name, there's a question as to whether constants should drop their C-ness
> (e.g. `C.C_BOOL` looks odd).
> Thought about this; C.C_BOOL and C.Win64.C_BOOL become C.BOOL,
> C.Win64.BOOL. That seems fine. Then there's also static import variants;
> C_BOOL, Win64.C_BOOL that go to BOOL and Win64.BOOL. I feel like for the
> C_BOOL -> BOOL case some information is lost, and that's the most common
> case we have it seems. But, maybe it's clear enough from the context
> that a C layout is being used? I think the main risk is confusing with
> Java carrier types or layouts (since the names are similar. Feeling a
> little on the fence about it, so I held off on the first revision.

I was about to mention when I wrote my message that, one counter argument to my proposal was that, if the code used a
static import at the top (and you have few examples in your patch) then the code would just use `BOOL`, which might
also be a bit thin.

In the end, perhaps this is tied with the support class called just `C`; e.g. `C.C_Bool` looks a tad weird,
`C<something>.C_BOOL` (e.g. `CSupport.C_BOOL`) perhaps not as much.

-------------

PR: https://git.openjdk.java.net/panama-foreign/pull/174