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

8241504: Expose MemoryLayout annotations/attributes in the public API #64

Closed
wants to merge 18 commits into from

Conversation

JornVernee
Copy link
Member

@JornVernee JornVernee commented Mar 24, 2020

Hi,

This PR exposed MemoryLayout attributes in the public API.

A summary of the changes:

  • Renamed annotation -> attribute everywhere to avoid confusion with Java language annotations
  • Made sure that annotations were being serialized as part of a MemoryLayout's ConstantDesc, as well as the alignment which was previously left out. (The latter also meant changing PaddingLayout::hasNaturalAlignment to always return true, so serializing alignment is skipped for padding).
  • Removed special casing for the abiType attribute. This can now instead be handled through the generic attribute mechanism.

Thanks,
Jorn


Progress

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

Issue

  • ⚠️ Temporary failure when trying to retrieve information on issue 8241504.

Reviewers

Download

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

@bridgekeeper
Copy link

bridgekeeper bot commented Mar 24, 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 Ready for review label Mar 24, 2020
@mlbridge
Copy link

mlbridge bot commented Mar 24, 2020

Webrevs

Copy link
Collaborator

@mcimadamore mcimadamore left a comment

Choose a reason for hiding this comment

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

Couple of suggestions, the rest looks really solid.

@JornVernee
Copy link
Member Author

I've also added some very basic tests of the new methods, mostly to sort of document the expectations w.r.t. how NAME is just another attribute, plus how using the attribute(String,Class) overload filters the optional with an instanceof check as opposed to simply casting (and potentially failing that cast with CCE).

Copy link
Collaborator

@mcimadamore mcimadamore left a comment

Choose a reason for hiding this comment

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

Some more comments

…ail with a CCE by casting directly if the type is incorrect, to avoid silent errors.
Copy link
Collaborator

@mcimadamore mcimadamore left a comment

Choose a reason for hiding this comment

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

Looks good

@openjdk
Copy link

openjdk bot commented Mar 26, 2020

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

8241504: Expose MemoryLayout annotations/attributes in the public API

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 /solves command.

➡️ To integrate this PR with the above commit message, type /integrate in a new comment.

@openjdk openjdk bot added the ready Ready to be integrated label Mar 26, 2020
@JornVernee
Copy link
Member Author

/integrate

@openjdk openjdk bot closed this Mar 26, 2020
@openjdk openjdk bot added integrated Pull request has been integrated and removed ready Ready to be integrated rfr Ready for review labels Mar 26, 2020
@openjdk
Copy link

openjdk bot commented Mar 26, 2020

@JornVernee
Pushed as commit 3eda917.

@mlbridge
Copy link

mlbridge bot commented Mar 26, 2020

Mailing list message from Jorn Vernee on panama-dev:

Changeset: 3eda917
Author: Jorn Vernee <jvernee at openjdk.org>
Date: 2020-03-26 13:19:18 +0000
URL: https://git.openjdk.java.net/panama-foreign/commit/3eda9176

8241504: Expose MemoryLayout annotations/attributes in the public API

Reviewed-by: mcimadamore

! src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/AbstractLayout.java
! src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/GroupLayout.java
! src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/MemoryLayout.java
! src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/MemoryLayouts.java
! src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/PaddingLayout.java
! src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/SequenceLayout.java
! src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/SystemABI.java
! src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/ValueLayout.java
! src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/Utils.java
! src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/abi/aarch64/CallArranger.java
! src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/abi/x64/sysv/CallArranger.java
! src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/abi/x64/windows/CallArranger.java
! src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/abi/x64/windows/Windowsx64ABI.java
! test/jdk/java/foreign/NativeTestHelper.java
+ test/jdk/java/foreign/TestLayoutAttributes.java
! test/jdk/java/foreign/TestLayoutConstants.java

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