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

8283175: Add equals/hashcode to MemorySegment #671

Closed

Conversation

mcimadamore
Copy link
Collaborator

@mcimadamore mcimadamore commented Mar 15, 2022

When looking at the API, we realized that MemorySegment is the only class not to expose equals/hashcode (MemoryLayout, MemoryAddress and MemorySession do). This makes it difficult to e.g. use a MemorySegment as a map key.

This patch adds equals/hashcode; the behavior is that of a shallow comparison - e.g. equals makes sure that the two segments point to the same memory region and have same spatial and temporal bounds. Read-only status is ignored by the comparison (on the basis that the segment is still pointing to the same memory region). And, since comparing memory session uses MemorySession::equals, segments backed by different "views" of the same session will also turn out to be equal (which also makes sense).

To perform a deeper comparison, clients can go through the MemorySegment::mismatch path, but I don't think that's a useful implementation for equals/hashCode (even though that's how byte buffers behave - but then, byte buffers do not provide a mismatch method).

When fixing this I realized that the equals method for FunctionDescriptor did not take into account the variable arity index, so I've fixed that too (and also tweaked the toString method to take that into account as well). Doing this revealed an issue in some of the calling convention tests, where we passed a variable arity descriptor as the expected descriptor, where in reality the calling convention "flattens" the variable arity descriptor into a specialized, fixed-arity one.


Progress

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

Issue

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 671

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

Using diff file

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

@bridgekeeper
Copy link

bridgekeeper bot commented Mar 15, 2022

👋 Welcome back mcimadamore! A progress list of the required criteria for merging this PR into foreign-preview 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 Ready for review label Mar 15, 2022
@mlbridge
Copy link

mlbridge bot commented Mar 15, 2022

Webrevs

Copy link
Member

@sundararajana sundararajana left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@openjdk
Copy link

openjdk bot commented Mar 15, 2022

@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:

8283175: Add equals/hashcode to MemorySegment

Reviewed-by: sundar

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-preview 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-preview branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready Ready to be integrated label Mar 15, 2022
@mcimadamore
Copy link
Collaborator Author

/integrate

@openjdk
Copy link

openjdk bot commented Mar 15, 2022

Going to push as commit 5b15ef7.

@openjdk openjdk bot added the integrated Pull request has been integrated label Mar 15, 2022
@openjdk openjdk bot closed this Mar 15, 2022
@openjdk openjdk bot removed ready Ready to be integrated rfr Ready for review labels Mar 15, 2022
@openjdk
Copy link

openjdk bot commented Mar 15, 2022

@mcimadamore Pushed as commit 5b15ef7.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integrated Pull request has been integrated
2 participants