Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rename SystemABI to ForeignLinker, and move C support to a separate class. #174

Closed
wants to merge 2 commits into from

Conversation

@JornVernee
Copy link
Member

@JornVernee JornVernee commented May 18, 2020

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


Progress

  • Change must not contain extraneous whitespace
  • Change must be properly reviewed

Reviewers

Download

$ git fetch https://git.openjdk.java.net/panama-foreign pull/174/head:pull/174
$ git checkout pull/174

- Create 'C' class to hold C constants and ForeignABI factory.
@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented May 18, 2020

👋 Welcome back jvernee! A progress list of the required criteria for merging this PR into foreign-abi will be added to the body of your pull request.

@openjdk openjdk bot added the rfr label May 18, 2020
@mlbridge
Copy link

@mlbridge mlbridge bot commented May 18, 2020

Webrevs

Copy link
Collaborator

@mcimadamore mcimadamore left a comment

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).
@mlbridge
Copy link

@mlbridge mlbridge bot commented May 18, 2020

Mailing list message from Jorn Vernee on panama-dev:

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
Yeah, that's a good suggestion. Will apply.
* 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 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).

Why not :) having more freedom to add small C utilities is also a good
reason to have a C/CSupport class. (I was using this class to put
VaList/VarArg support in in my experiments as well). Also, looking at
the space occupied by the JNI API, I think that we can afford some API
surface to add some convenience utilities for C in Panama.

Jorn

@mcimadamore
Copy link
Collaborator

@mcimadamore mcimadamore commented May 18, 2020

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.

@mcimadamore
Copy link
Collaborator

@mcimadamore mcimadamore commented May 18, 2020

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
Yeah, that's a good suggestion. Will apply.

The Support suffix is something that we use sometimes in the JDK (e.g. StreamSupport). If you think that C in CSupport is not much, I thought we could tweak to ClangSupport or something like that but that will likely lead to confusion (as it parses as clang).

@JornVernee
Copy link
Member Author

@JornVernee JornVernee commented May 18, 2020

The Support suffix is something that we use sometimes in the JDK (e.g. StreamSupport). If you think that C in CSupport is not much, I thought we could tweak to ClangSupport or something like that but that will likely lead to confusion (as it parses as clang).

Uh, yeah, I just had to do a double take on that 😄

I think CSupport looks fine as a name though.

@JornVernee
Copy link
Member Author

@JornVernee JornVernee commented May 18, 2020

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.

I agree, it doesn't look as weird after applying the rename to CSupport locally.

@mcimadamore
Copy link
Collaborator

@mcimadamore mcimadamore commented May 18, 2020

Renaming looks good - is there a reason as to why the Cstring was left in place as a test util? If you prefer to address as a followup that's ok with me.

@openjdk
Copy link

@openjdk openjdk bot commented May 18, 2020

@JornVernee This change now passes all automated pre-integration checks, type /integrate in a new comment to proceed. After integration, the commit message will be:

Rename SystemABI to ForeignLinker, and move C support to a separate class.

Reviewed-by: mcimadamore
  • If you would like to add a summary, use the /summary command.
  • To credit additional contributors, use the /contributor command.
  • To add additional solved issues, use the /issue command.

Since the source branch of this PR was last updated there have been 7 commits pushed to the foreign-abi branch:

  • f0c2d3a: Automatic merge of foreign-memaccess into foreign-abi
  • 0bc9c6b: Move MemoryAddress::copy (more review feedback)
  • 90a47d1: Move MemoryAddress::copy (ABI version)
  • d4ce7aa: Automatic merge of foreign-memaccess into foreign-abi
  • 3487383: Move MemoryAddress::copy
  • c8df5aa: Automatic merge of foreign-memaccess into foreign-abi
  • 928a8af: Move "owner" field and thread-confinement checks to MemoryScope

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid automatic rebasing, please merge foreign-abi into your branch, and then specify the current head hash when integrating, like this: /integrate f0c2d3abda80f5fb44253b5386a5f6acd54a7319.

➡️ To integrate this PR with the above commit message to the foreign-abi branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready label May 18, 2020
@JornVernee
Copy link
Member Author

@JornVernee JornVernee commented May 18, 2020

Renaming looks good - is there a reason as to why the Cstring was left in place as a test util? If you prefer to address as a followup that's ok with me.

Ah, you were talking about the test util. I was thinking about the CString class that is generated by jextract :)

I'd rather address that separately, since I think I have some other ideas w.r.t. Cstring I'd also like to bring up.

@JornVernee
Copy link
Member Author

@JornVernee JornVernee commented May 18, 2020

I'll do a rebase and re-run the tests before integrating, just to be sure.

@JornVernee
Copy link
Member Author

@JornVernee JornVernee commented May 18, 2020

/integrate

@openjdk openjdk bot closed this May 18, 2020
@openjdk openjdk bot added integrated and removed ready labels May 18, 2020
@openjdk
Copy link

@openjdk openjdk bot commented May 18, 2020

@JornVernee The following commits have been pushed to foreign-abi since your change was applied:

  • f0c2d3a: Automatic merge of foreign-memaccess into foreign-abi
  • 0bc9c6b: Move MemoryAddress::copy (more review feedback)
  • 90a47d1: Move MemoryAddress::copy (ABI version)
  • d4ce7aa: Automatic merge of foreign-memaccess into foreign-abi
  • 3487383: Move MemoryAddress::copy
  • c8df5aa: Automatic merge of foreign-memaccess into foreign-abi
  • 928a8af: Move "owner" field and thread-confinement checks to MemoryScope

Your commit was automatically rebased without conflicts.

Pushed as commit eebb9a2.

@openjdk openjdk bot removed the rfr label May 18, 2020
@JornVernee JornVernee deleted the JornVernee:ForeignABI branch May 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.