Skip to content

Conversation

fparain
Copy link
Collaborator

@fparain fparain commented May 9, 2025

Since the removal of Q-types and the notion that nullability was not part of the Java type, there was an awkward situation because nullable arrays of value types and null free arrays of value types had each a different Java mirror when they were in fact supposed to have the same Java type.
In order to accommodate to the new situation, that arrays can have properties (nullability, flatness, atomicity, etc.) that are not part of their Java type, the 1-1 relationship between the *ArrayKlass and the Java mirror must be broken.
The proposed solution is to dedicate one instance of ObjArrayKlass to represent the Java type of the array in the JVM, and have this instance being the counterpart of the Java mirror of the array, and have several instances of RefArrayKlass and FlatArrayKlass that represent the refinements of the Java array type. Each RefArrayKlass/FlatArrayKlass encodes the characteristic of a Java array for a given element type and a set of properties.


Progress

  • Change must not contain extraneous whitespace

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/valhalla.git pull/1452/head:pull/1452
$ git checkout pull/1452

Update a local copy of the PR:
$ git checkout pull/1452
$ git pull https://git.openjdk.org/valhalla.git pull/1452/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1452

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/valhalla/pull/1452.diff

@bridgekeeper
Copy link

bridgekeeper bot commented May 9, 2025

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

@openjdk
Copy link

openjdk bot commented May 9, 2025

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

Copy link
Contributor

@coleenp coleenp left a comment

Choose a reason for hiding this comment

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

Makes sense - found a typo. Are you going to check this in with UseNewCode2 or you could add a global flag for this because I think you're going to need a new one?


class ClassLoaderData;

// RefjArrayKlass is the klass for arrays of references
Copy link
Contributor

Choose a reason for hiding this comment

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

you have an extra j in this.

private:
// Constructor
RefArrayKlass(int n, Klass *element_klass, Symbol *name, bool null_free);
static RefArrayKlass *allocate(ClassLoaderData *loader_data, int n, Klass *k, Symbol *name, bool null_free, TRAPS);
Copy link
Contributor

Choose a reason for hiding this comment

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

The stars go with the types, ie ClassLoaderData* loader_data.

@openjdk
Copy link

openjdk bot commented May 21, 2025

@fparain this pull request can not be integrated into lworld due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout array_klasses
git fetch https://git.openjdk.org/valhalla.git lworld
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge lworld"
git push

@openjdk openjdk bot added the merge-conflict Pull request has merge conflict with target branch label May 21, 2025
@openjdk openjdk bot removed the merge-conflict Pull request has merge conflict with target branch label May 22, 2025
@bridgekeeper
Copy link

bridgekeeper bot commented Jul 5, 2025

@fparain This pull request has been inactive for more than 8 weeks and will be automatically closed if another 8 weeks passes without any activity. To avoid this, simply issue a /touch or /keepalive command to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

@fparain
Copy link
Collaborator Author

fparain commented Jul 9, 2025

/keepalive

@openjdk
Copy link

openjdk bot commented Jul 9, 2025

@fparain The pull request is being re-evaluated and the inactivity timeout has been reset.

@openjdk openjdk bot added the merge-conflict Pull request has merge conflict with target branch label Jul 30, 2025
@openjdk
Copy link

openjdk bot commented Jul 31, 2025

⚠️ @fparain This pull request contains merges that bring in commits not present in the target repository. Since this is not a "merge style" pull request, these changes will be squashed when this pull request in integrated. If this is your intention, then please ignore this message. If you want to preserve the commit structure, you must change the title of this pull request to Merge <project>:<branch> where <project> is the name of another project in the OpenJDK organization (for example Merge jdk:master).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge-conflict Pull request has merge conflict with target branch
Development

Successfully merging this pull request may close these issues.

5 participants