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

8264434: Remove ResourceScope-less overload from API #485

Conversation

@mcimadamore
Copy link
Collaborator

@mcimadamore mcimadamore commented Mar 30, 2021

This patch does a bunch of API cleanups related to overloaded methods:

  • Remove ResouceScope-less overloads (in MemorySegment, CLinker, MemoryAddress, VaList)
  • Remove ResourceScope factory overloads which take Object attachment (in ResourceScope)
  • Fixup javadoc

The changes are mostly mechanical - e.g. an extra ResourceScope.ofImplicit() is added wherever needed.
Similarly, for address to segment conversion, an extra ResourceScope.globalScope() is added.

The term implicit has been selected to denote the subset of scopes that are not only managed by a Cleaner, but that do not play into explicit deallocation.

So, ResourceScope.ofImplicit() returns a new implicit scope, whereas ResourceScope.globalScope, returns a constant implicit scope which is assumed to be alive for the entire duration of the application.

A new predicate has been added on ResourceScope, namely isImplicit which allows client to see whether a segment supports deterministic closure (we used to have a similar predicate, but was called isCloseable and that created confusion w.r.t. the semantics of close() - the new naming is more apt, in that it clearly reflects a permanent property of the resource scope).


Progress

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

Issue

  • JDK-8264434: Remove ResourceScope-less overload from API

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/panama-foreign pull/485/head:pull/485
$ git checkout pull/485

Update a local copy of the PR:
$ git checkout pull/485
$ git pull https://git.openjdk.java.net/panama-foreign pull/485/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 485

View PR using the GUI difftool:
$ git pr show -t 485

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/panama-foreign/pull/485.diff

* Remove ResourceScope factory overloads which take Object attachment
* Fixup javadoc
@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Mar 30, 2021

👋 Welcome back mcimadamore! A progress list of the required criteria for merging this PR into foreign-memaccess+abi will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk openjdk bot added the rfr label Mar 30, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Mar 30, 2021

Webrevs

Copy link
Member

@PaulSandoz PaulSandoz left a comment

Looking at the changes in the tests it occurred to me that Resource.ofX does not work so well with a static import, Resource.XScope would work better and is consistent with globalScope.

May be worth reviewing factory methods of other classes too, namely SegmentAllocator.

For common cases i would imagine static imports may be useful, so the method names should stand on their own, at the expense of a few more characters (3 for scope) when the method is qualified.

Copy link
Member

@JornVernee JornVernee left a comment

LGTM. I agree with Paul that it would be nice to rename ofXYZ factories to something that is more amenable to static importing (as also discussed offline), e.g. ResourceScope.ofImplicit() -> ResourceScope.implicit() to be consistent with SegmentAllocator.implicit().

@openjdk
Copy link

@openjdk openjdk bot commented Mar 30, 2021

@mcimadamore This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8264434: Remove ResourceScope-less overload from API

Reviewed-by: psandoz, jvernee

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been no new commits pushed to the foreign-memaccess+abi branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential automatic rebasing, please check the documentation for the /integrate command for further details.

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

@openjdk openjdk bot added the ready label Mar 30, 2021
@mcimadamore
Copy link
Collaborator Author

@mcimadamore mcimadamore commented Mar 30, 2021

Looking at the changes in the tests it occurred to me that Resource.ofX does not work so well with a static import, Resource.XScope would work better and is consistent with globalScope.

Ha! I had exactly this conversation earlier (offline).

Same applied to MemoryLayout.ofStruct and others.

I'm happy to rename all this, but (i) I'd like to separate from this change and (ii) I need some guidance from JDK API gurus - e.g. some JDK factories do use ofXYZ (e.g. List.of, ClassDesc.of) is there some rule about naming which will land the API in a more consistent place with the rest of the JDK?

May be worth reviewing factory methods of other classes too, namely SegmentAllocator.

For common cases i would imagine static imports may be useful, so the method names should stand on their own, at the expense of a few more characters (3 for scope) when the method is qualified.

mcimadamore and others added 3 commits Mar 30, 2021
…ResourceScope.java

Co-authored-by: Jorn Vernee <JornVernee@users.noreply.github.com>
…ResourceScope.java

Co-authored-by: Paul Sandoz <paul.d.sandoz@googlemail.com>
…ResourceScope.java

Co-authored-by: Paul Sandoz <paul.d.sandoz@googlemail.com>
@mcimadamore
Copy link
Collaborator Author

@mcimadamore mcimadamore commented Mar 30, 2021

LGTM. I agree with Paul that it would be nice to rename ofXYZ factories to something that is more amenable to static importing (as also discussed offline), e.g. ResourceScope.ofImplicit() -> ResourceScope.implicit() to be consistent with SegmentAllocator.implicit().

I think whatever naming we choose, we need to have some visual distinction between factories which are just "accessors" e.g. ResourceScope.globalScope and things which create a new scope e.g. ResourceScope.ofImplicit. But, as discussed offline, I don't want to go down the road of calling the latter makeImplicitScope or newImplicitScope.

@mcimadamore
Copy link
Collaborator Author

@mcimadamore mcimadamore commented Mar 30, 2021

/integrate

@openjdk openjdk bot closed this Mar 30, 2021
@openjdk openjdk bot added integrated and removed ready labels Mar 30, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Mar 30, 2021

@mcimadamore Pushed as commit a4ba76c.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

@openjdk openjdk bot removed the rfr label Mar 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants