8277015: Use blessed modifier order in Panama code #6355

magicus · 2021-11-11T14:50:43Z

I ran bin/blessed-modifier-order.sh on source owned by Project Panama. This scripts verifies that modifiers are in the "blessed" order, and fixes it otherwise. I have manually checked the changes made by the script to make sure they are sound.

In this case, while the script did into the "correct" thing, it turns out that the method signatures in src/jdk.incubator.vector/share/classes/jdk/incubator/vector has some room for improvement... The files contains method headers which look like this:

        final @Override
        @ForceInline
        long longToElementBits(...

        @ForceInline
        static long toIntegralChecked(...

        @ForceInline
        @Override final
        ByteVector dummyVector(...

My personal opinion is that these should have been written like this:

        @Override
        @ForceInline
        final long longToElementBits(...

        @ForceInline
        static long toIntegralChecked(...

        @ForceInline
        @Override
        final ByteVector dummyVector(...

or possibly

        @Override @ForceInline
        final long longToElementBits(...

        @ForceInline
        static long toIntegralChecked(...

        @ForceInline @Override
        final ByteVector dummyVector(...

If you want me to make that change as well as part of the fix, let me know.

Furthermore, I don't know how much the code in mainline differs from the code in the Panama branches. If the discrepancy is large, you might want to run bash bin/blessed-modifier-order.sh src/jdk.incubator.vector and bash bin/blessed-modifier-order.sh src/jdk.incubator.foreign in those branches.

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed

Issue

JDK-8277015: Use blessed modifier order in Panama code

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/6355/head:pull/6355
$ git checkout pull/6355

Update a local copy of the PR:
$ git checkout pull/6355
$ git pull https://git.openjdk.java.net/jdk pull/6355/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 6355

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/6355.diff

bridgekeeper · 2021-11-11T14:51:46Z

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

openjdk · 2021-11-11T14:53:32Z

@magicus The following label will be automatically applied to this pull request:

core-libs

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

mlbridge · 2021-11-11T14:57:58Z

Webrevs

00: Full (c5087a9d)

mlbridge · 2021-11-11T15:05:04Z

Mailing list message from Magnus Ihse Bursie on core-libs-dev:

I assume this is relevant for panama-dev as well, but I can't
unfortunately tag the issue as such in Github/Skara. (Maybe we should
have a mapping for jdk.incubator.vector/foreign -> panama-dev?)

/Magnus

On 2021-11-11 15:57, Magnus Ihse Bursie wrote:

mlbridge · 2021-11-11T15:33:04Z

Mailing list message from Maurizio Cimadamore on core-libs-dev:

Hi Magnus,
as we're in the process of integrating new API changes (see [1]) - would
it be possible to postpone this? I'd like to minimize merge conflicts,
if that makes sense.

Thanks
Maurizio

[1] - https://github.com//pull/5907

On 11/11/2021 14:57, Magnus Ihse Bursie wrote:

mcimadamore

On a second look, changes are relatively contained. I'm ok with integrating this ahead of the Panama/foreign refresh. I found a possible issue in the vector API, with public being of the same line as the annotation, which looks odd (as you have noticed).

mcimadamore · 2021-11-11T15:47:11Z

src/jdk.incubator.vector/share/classes/jdk/incubator/vector/ByteVector.java

@@ -3905,8 +3905,8 @@ final ByteVector fromIntValues(int[] values) {
        // Virtual constructors

        @ForceInline
-        @Override final
-        public ByteVector fromArray(Object a, int offset) {
+        @Override public


public should probably go on a different line? (same as for the ones that follow)

Note these files are generated from a template X-Vector.java.template and the script gen-src.sh (not yet integrated into the build system). So the template requires updating, from which the script is run to generate the source. We can do this after integration of #5873

magicus · 2021-11-11T15:58:06Z

This is by no means urgent. If it's more convenient for you to wait until after the refresh, I can certainly do so.

mcimadamore · 2021-11-11T16:45:40Z

This is by no means urgent. If it's more convenient for you to wait until after the refresh, I can certainly do so.

I guess there is a chance that after vector and foreign are re-incubated in 18, we might need to do this again.

magicus · 2021-11-11T20:22:25Z

You could also do this directly in the Panama repo branches. I'll volunteer to help, if you want.

mcimadamore · 2021-11-11T21:34:17Z

You could also do this directly in the Panama repo branches. I'll volunteer to help, if you want.

I'll run the script on the PR I've submitted for the Foreign API, and I will update that one - thanks. Perhaps @PaulSandoz can do the same for the Vector API refresh?

PaulSandoz · 2021-11-11T21:40:18Z

I would prefer to leave the Vector API PR as is, we are getting close to integration, and apply such changes after integration as a separate commit on the mainline.

mcimadamore · 2021-11-12T11:13:24Z

You could also do this directly in the Panama repo branches. I'll volunteer to help, if you want.

I'll run the script on the PR I've submitted for the Foreign API, and I will update that one - thanks. Perhaps @PaulSandoz can do the same for the Vector API refresh?

Done - the jdk.incubator.foreign changes are now part of #5907

magicus · 2021-11-12T14:13:14Z

@mcimadamore Thanks!

@PaulSandoz I'll keep this PR open until after the integration is done, and check if or what parts are still needed after that. I don't want to mess up your integration with trivia like this. (Had I realized the bad timing I would have waited before opening this PR)

PaulSandoz · 2021-11-12T17:01:38Z

@mcimadamore Thanks!

@PaulSandoz I'll keep this PR open until after the integration is done, and check if or what parts are still needed after that. I don't want to mess up your integration with trivia like this. (Had I realized the bad timing I would have waited before opening this PR)

Thanks, integration should happen to today, then i can help guide you on what to update.

magicus · 2021-11-17T12:41:30Z

@PaulSandoz @mcimadamore Would you be ok with me pushing this now?

mcimadamore · 2021-11-17T13:40:39Z

@PaulSandoz @mcimadamore Would you be ok with me pushing this now?

As the foreign API bits have been made part of the upcoming foreign PR refresh, I think this patch should perhaps avoid changes inside jdk.incubator.foreign (to avoid potential conflicts).

magicus · 2021-11-17T14:56:19Z

So the refresh is still upcoming? Sorry, I thought it was pushed last week. When it is pushed, I'll merge in master and the duplicated changes will just be removed from the PR.

mcimadamore · 2021-11-17T15:11:24Z

So the refresh is still upcoming? Sorry, I thought it was pushed last week. When it is pushed, I'll merge in master and the duplicated changes will just be removed from the PR.

There are two refreshes:

foreign API refresh: https://git.openjdk.java.net/jdk/pull/5907
vector API refresh: https://git.openjdk.java.net/jdk/pull/5873

Your patch covers both vector API and foreign API, but these APIs have independent JEPs so their integration schedule differs. The vector API refresh was integrated just few days ago, while the foreign API refresh will hopefully be integrated soon (but JEP for that is still not targeted to 18).

PaulSandoz · 2021-11-17T19:21:39Z

@PaulSandoz @mcimadamore Would you be ok with me pushing this now?

Specifically for the *Vector.java classes you to modify the X-Vector.java.template.

Then run gen-src.sh which will generate the *Vector.java. We need to integrate this into the build system but it always gets bumped down in priority.

magicus · 2021-11-17T19:59:08Z

@mcimadamore I see. Sorry for my confusion. :-( Maybe I should split this in two PRs, then...

@PaulSandoz Oh, I was not even aware of that. That sounds like quite an easy fix for me. Is there an JBS issue on this?

PaulSandoz · 2021-11-17T20:14:08Z

@PaulSandoz Oh, I was not even aware of that. That sounds like quite an easy fix for me. Is there an JBS issue on this?

There is now :-) I just created one:

https://bugs.openjdk.java.net/browse/JDK-8277349

bridgekeeper · 2021-12-15T23:32:24Z

@magicus This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

bridgekeeper · 2022-01-13T01:53:15Z

@magicus This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the /open pull request command.

8277015: Use blessed modifier order in Panama code

c5087a9

openjdk bot added the rfr Pull request is ready for review label Nov 11, 2021

openjdk bot added the core-libs core-libs-dev@openjdk.org label Nov 11, 2021

mcimadamore reviewed Nov 11, 2021

View reviewed changes

bridgekeeper bot closed this Jan 13, 2022

magicus deleted the panama-blessed-order branch March 15, 2022 13:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8277015: Use blessed modifier order in Panama code #6355

8277015: Use blessed modifier order in Panama code #6355

magicus commented Nov 11, 2021 •

edited by openjdk bot

Loading

bridgekeeper bot commented Nov 11, 2021

openjdk bot commented Nov 11, 2021

mlbridge bot commented Nov 11, 2021

mlbridge bot commented Nov 11, 2021

mlbridge bot commented Nov 11, 2021

mcimadamore left a comment •

edited

Loading

mcimadamore Nov 11, 2021

PaulSandoz Nov 11, 2021

magicus commented Nov 11, 2021

mcimadamore commented Nov 11, 2021

magicus commented Nov 11, 2021

mcimadamore commented Nov 11, 2021

PaulSandoz commented Nov 11, 2021

mcimadamore commented Nov 12, 2021

magicus commented Nov 12, 2021

PaulSandoz commented Nov 12, 2021

magicus commented Nov 17, 2021

mcimadamore commented Nov 17, 2021

magicus commented Nov 17, 2021

mcimadamore commented Nov 17, 2021

PaulSandoz commented Nov 17, 2021

magicus commented Nov 17, 2021

PaulSandoz commented Nov 17, 2021

bridgekeeper bot commented Dec 15, 2021

bridgekeeper bot commented Jan 13, 2022

8277015: Use blessed modifier order in Panama code #6355

8277015: Use blessed modifier order in Panama code #6355

Conversation

magicus commented Nov 11, 2021 • edited by openjdk bot Loading

Progress

Issue

Reviewing

bridgekeeper bot commented Nov 11, 2021

openjdk bot commented Nov 11, 2021

mlbridge bot commented Nov 11, 2021

Webrevs

mlbridge bot commented Nov 11, 2021

mlbridge bot commented Nov 11, 2021

mcimadamore left a comment • edited Loading

Choose a reason for hiding this comment

mcimadamore Nov 11, 2021

Choose a reason for hiding this comment

PaulSandoz Nov 11, 2021

Choose a reason for hiding this comment

magicus commented Nov 11, 2021

mcimadamore commented Nov 11, 2021

magicus commented Nov 11, 2021

mcimadamore commented Nov 11, 2021

PaulSandoz commented Nov 11, 2021

mcimadamore commented Nov 12, 2021

magicus commented Nov 12, 2021

PaulSandoz commented Nov 12, 2021

magicus commented Nov 17, 2021

mcimadamore commented Nov 17, 2021

magicus commented Nov 17, 2021

mcimadamore commented Nov 17, 2021

PaulSandoz commented Nov 17, 2021

magicus commented Nov 17, 2021

PaulSandoz commented Nov 17, 2021

bridgekeeper bot commented Dec 15, 2021

bridgekeeper bot commented Jan 13, 2022

magicus commented Nov 11, 2021 •

edited by openjdk bot

Loading

mcimadamore left a comment •

edited

Loading